Community
Participate
Working Groups
The EFS API has predefined error codes in EFS.ERROR_* but RSEFileStore currently doesn't set these when reporting errors. Also, there are subtle differences in error reporting, e.g. EFS Localstore doesn't throw an error when trying to delete a non-existing file, but RSE does. In order to find those places where RSE differs from the native EFS Localstore implementation, I wrote a unittest (RSEFileStoreTest), which runs some EFS testcases on native Localstore first, and then on the RSE "Local" filesystem. Other connection types (ssh, dstore, ftp) can be enabled in the test. By running the unittest, I found and fixed some subtle differences, see attachment. The trick is to correctly interpret the kinds of exceptions that RSE sends as per bug 226374, especially SystemElementNotFoundException.
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.
