Community
Participate
Working Groups
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.
But you won't change IFile#setContents(*), right? PW
(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.
Moving to 3.8. Szymon P., feel free to move it back to 3.7, if you have time to fix it.
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.
James, since you were initially working on bug 332543, could you please review?
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.
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.
Fixed in master: http://git.eclipse.org/c/platform/eclipse.platform.resources.git/commit/?id=2ef48c2ee33b3f62226245def5b509579921a511
