Summary: | [efs][api] Fix error reporting codes from RSE EFS Provider | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | [Tools] Target Management | Reporter: | Martin Oberhuber <mober.at+eclipse> | ||||||
Component: | RSE | Assignee: | Martin Oberhuber <mober.at+eclipse> | ||||||
Status: | RESOLVED FIXED | QA Contact: | Martin Oberhuber <mober.at+eclipse> | ||||||
Severity: | normal | ||||||||
Priority: | P3 | CC: | rgerganov | ||||||
Version: | 3.0 | Keywords: | api | ||||||
Target Milestone: | 3.0 RC2 | Flags: | rgerganov:
review+
|
||||||
Hardware: | PC | ||||||||
OS: | Windows XP | ||||||||
Whiteboard: | |||||||||
Bug Depends on: | 226374, 230919 | ||||||||
Bug Blocks: | 218643, 234000, 233651 | ||||||||
Attachments: |
|
Description
Martin Oberhuber
2008-05-26 13:45:24 EDT
Created attachment 102015 [details]
Patch fixing the issues
Attached patch fixes the issues to make the "Local" FileService consistent with EFS native LocalStore:
* Created RSEFileStoreImpl#rethrowCoreException() to take a hint for an error
code to use in case anything goes wrong
* Use and correctly interpret SystemElementNotFoundException where needed
* LocalFileService: Evaluate the OS's error message on failed "chmod" or
"attrib -r" execution and put into 2nd level error details message. This code
should also go into DStore UniversalFileSystemMiner!
* LocalFileService: Correctly interpret FileNotFoundException in case of
creating Streams
In order to properly convey all information in SystemElementNotFoundException, I had to add some constructors to it (i.e. to keep the right plugin ID from the plugin that issued the error; or, to be able and subclass SystemElementNotFoundException for more specific error messages like "File not Found" in the future.
Patch committed: [233993] Improve EFS error reporting Rado can you please review on sync? Adding [api] tag since there are new constructors in SystemElementNotFoundException. Created attachment 102072 [details] Additional patch for error reporting Attached additional patch is needed for improved error reporting, and in order to fix "deep" folder creation while bug 234026 (IRemoteFileSubSystem.createFolders()) is not yet properly fixed. Additional patch committed, Rado please review both of them. JUnit tests run all fine with these fixes. (RSEFileStoreTest.java) Patches look good to me and most importantly you made unit tests, so we can find another similar problems with other filesystems besides Local. I also like the approach for converting RSE exceptions to EFS errors. Here are some minor issues reported by FindBugs for the LocalFileService: - return values ignored for java.io.File#(mkdirs|setLastModified|delete) -- these should be always checked and the proper exception must be thrown immediately. Please note how easy is to forget checking the returned boolean value, that's why I always prefer to use exceptions. - remoteError = ": " + new BufferedReader(new InputStreamReader(p.getErrorStream())) in LocalFileService#setReadOnly() -- the stream is never closed and this may result in file descriptor leak. Thanks for the review! > - return values ignored for java.io.File#(mkdirs|setLastModified|delete) -- > these should be always checked and the proper exception must be thrown You're right. I actually found some similar issues with the unit tests and did convert some of these into throwing exceptions. The missing places should be addressed. > - remoteError = ": " + new BufferedReader(new > InputStreamReader(p.getErrorStream())) in LocalFileService#setReadOnly() -- the > stream is never closed and this may result in file descriptor leak. Hm... I also thought about this issue, but then I had the impression that the Garbage Collector should clean these up: http://journals.ecs.soton.ac.uk/java/tutorial/java/io/overview.html "You can explicitly close an output stream with the close method, or let it be closed implicitly when the OutputStream is garbage collected, which occurs when the object is no longer referenced." I understand your concern, and doing an explicit close certainly won't hurt... but I'd still like to know for sure, so if you have any reference that has a "best practice" for handling of such temporary Streams? (In reply to comment #7) > Thanks for the review! > > > - return values ignored for java.io.File#(mkdirs|setLastModified|delete) -- > > these should be always checked and the proper exception must be thrown > > You're right. I actually found some similar issues with the unit tests and did > convert some of these into throwing exceptions. The missing places should be > addressed. > line 288, destinationParent.mkdirs(), LocalFileService#upload() line 395, parentDir.mkdirs(), LocalFileService#download() line 476, destinationFile.setLastModified(..), LocalFileService#download() line 583, destinationParent.mkdirs(), LocalFileService#upload() line 661, destinationFile.setLastModified(..), LocalFileService#upload() line 879, parentFile.mkdirs(), LocalFileService#createFile() line 1411, destinationFile.delete(), LocalFileService#copyFromArchive() Maybe we should open another bug for those? > > - remoteError = ": " + new BufferedReader(new > > InputStreamReader(p.getErrorStream())) in LocalFileService#setReadOnly() -- the > > stream is never closed and this may result in file descriptor leak. > > Hm... I also thought about this issue, but then I had the impression that the > Garbage Collector should clean these up: > http://journals.ecs.soton.ac.uk/java/tutorial/java/io/overview.html > "You can explicitly close an output stream with the close method, or let it > be closed implicitly when the OutputStream is garbage collected, which > occurs when the object is no longer referenced." > > I understand your concern, and doing an explicit close certainly won't hurt... > but I'd still like to know for sure, so if you have any reference that has a > "best practice" for handling of such temporary Streams? > Well, this is definitely a problem on most of the embedded JVMs but I am not sure about the desktop ones. I guess you are right and the streams could be implicitly closed by the GC. |