Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 334081

Summary: Input and output streams should be closed by their creators
Product: [Eclipse Project] Platform Reporter: Szymon Brandys <Szymon.Brandys>
Component: ResourcesAssignee: Szymon Ptaszkiewicz <sptaszkiewicz>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: jamesblackburn+eclipse, pwebster
Version: 3.7   
Target Milestone: 4.4 M6   
Hardware: PC   
OS: Linux-GTK   
Whiteboard:
Bug Depends on: 332543    
Bug Blocks:    
Attachments:
Description Flags
Patch v01 none

Description Szymon Brandys CLA 2011-01-12 08:00:40 EST 
This came up to me while reviewing the fix for bug 332543. I noticed that some methods in core.resources closes streams that are passed in. A better practice is to get callers to close streams. 

For instance:
SaveManager#writeTree(Project project, DataOutputStream output, IProgressMonitor monitor) closes the output stream in the finally block. Similar method writeTree(Map statesToSave, DataOutputStream output, IProgressMonitor monitor) leaves it to the caller. BTW the first method is called by two other SaveManager methods only, and these two methods also close the stream.

Another places I found are:
- FileStore#transferStreams
- FileUtil#transferStreams

SafeFileOutputStream#transferStreams does not close the passed streams.

It looks like a quite easy refactoring.
Comment 1 Paul Webster CLA 2011-01-12 08:46:44 EST 
But you won't change IFile#setContents(*), right?

PW
Comment 2 Szymon Brandys CLA 2011-01-12 08:52:36 EST 
(In reply to comment #1)
> But you won't change IFile#setContents(*), right?
> 
> PW

No. I was thinking about the mentioned internal methods. I noticed there are some inconsistencies like in #writeTree and some streams are closed multiple times.
Comment 3 Szymon Brandys CLA 2011-03-24 06:37:07 EDT 
Moving to 3.8. Szymon P., feel free to move it back to 3.7, if you have time to fix it.
Comment 4 Szymon Ptaszkiewicz CLA 2011-06-10 07:58:37 EDT 
Created attachment 197766 [details]
Patch v01

API methods still close the streams that are passed in. In all other places that I found, streams are closed by creators.
Comment 5 Szymon Ptaszkiewicz CLA 2011-06-10 08:00:02 EDT 
James, since you were initially working on bug 332543, could you please review?
Comment 6 James Blackburn CLA 2011-07-01 13:44:18 EDT 
Hi SzymonP, Sorry for the delay (I was on holiday).

Looking at this patch I'm not sure whether the reasoning behind this change is right.  From my POV it complicates the error handling, increases loc, and may make it more likely that streams aren't correctly closed in the future. 

In core.filesystem:
 - Taking the private helper method FileStore#transferStreams.  transferStreams wraps and categorizes the IOException and rethrows a CoreException.  By moving the close() to the caller, an additional layer of try { } catch nesting has been added. 

In core.resources:
- FSRM#write now has to do additional error handling for close (much like core.fs above).
- In ModelObjectWriter & SaveManager#writeTree you move the output stream close() from the callers. This seems to go against having streams closed by their creators?

Having applied the patch its no longer straightforward to reason about whether all the streams are safely close()d.  I'm finding myself using call-hierarchy and examining the call-tree for potential bugs.  The existing implementation centralises the logic and seems clearer to me.

Given FileUtil#transferStreams is an internal helper method, I think it makes sense that it wraps up all the error handling.  I personally don't see a problem with it also closing the streams, but the behaviour, whatever it is, should be documented in JavaDoc.
Comment 7 Szymon Ptaszkiewicz CLA 2011-07-13 08:29:11 EDT 
Thanks James. Your are right that in most cases we can see increased complexity of the code. I will adjust the patch to add some comments in places where changing the current implementation is not reasonable.