Community
Participate
Working Groups
Currently in DStoreFileService(IFileService) there is no way to tell what the individual results of the batch/multiple methods are (copyBatch, deleteBatch, uploadMultiple, downloadMultiple). I am trying to manage a set of bidi properties and need to be able to tell which (if any) individual operations where successful, rather than the entire operation as a single unit.
One approach for these multi-operations may be to change the result from a boolean to an array of booleans however I think we typically throw an exception when the first such operation fails. This brings up the question whether other operations should continue when one of them has failed or not. If we agree that if any operation fails, we should throw an exception, then I expect this is working as expected if not, then we'll probably need to change the IFileService APIs a bit. Any opinions?
I think that up to now, our take on this was that if any operation fails on the remote system, it's often unclear what the state of the entire system is so it's better to have the client query the state of the remote system again (i.e. flush all caches and assume that nothing is known at all about the remote). This would imply that an exception is thrown when the first failure occurs during a batch operation, and upon receiving such an exception the client needs to query the status (i.e. perform a list query) on the objects in question. But Javadocs in IFileService are not really excplicit on this, so the least that we definitely need to do is clarify the situation in the Javadocs. My personal take varies regarding the various queries: * listMultiple getFileMultiple These are non-intrusive on the remote, so I think we should find a way of returning valid results for those parts of the query that worked, and a status for those that did not work. In fact we could even be API compatible if we declare public class HostFileStatus implements IHostFile { SystemMessageException getStatus(); boolean exists() { return false; } boolean isDirectory() { return false; } boolean isFile() { return false; } boolean isArchive() { return false; } } then from the IHostFile[] array returned, valid results are normal IHostFile objects whereas those queries that errored are the HostFileStatus objects. But I'm open to other API ideas. I think we cannot use a MultiStatus because we don't want eclipse.core dependency in the Services. In order to make the code simpler, we could even extend IHostFile interface adding a getStatus() method which would return null on valid files and non-null on invalid results (which are in fact messages) * uploadMultiple/downloadMultiple: I'm OK with exception on first error, since we don't save a lot of time on these operations anyways. We could return a kind of Status with the exception that explains what parts of the operation were successful, though I'm not exactly sure about the value of such addition. Another option especially for uploadMultiple would be to return IHostFile[] array just like the getFileMultiple call. But then, exactly the same result could be obtained with try { uploadMultiple(...) } catch(Exception e) { return getFileMultiple(...) } * deleteBatch: is very intrusive on the remote so I think it's better to bail out with exception on the first error, and have the client query the result again rather than returning a multi-status.
+1 for making the IFileService javadocs more explicit
Dave D, I'm reassigning to you as per the javadoc discussion in the last meeting.
Since this is a javadoc issue, I'll schedule it just after M6.
All batch operations are now documented to stop at the first error. Please review the committed IFileService javadoc.
The doc now says for listMultiple(): If an error occurs during the retrieval of the contents of a folder, this operation stops on that folder and a {@link SystemMessageException} is thrown. Items retrieved before that folder will be returned. I wonder how that should work technically if an exception is thrown? The result of the method is passed by return value, which is not available in case of an exception. Is this a reason for actually changing the method specification, and having the client pass in an empty List that the method populates with the IHostFile elements to be retrieved? I think we can still do that, since the API is new in RSE 3.0 so it's not to be considered a breaking change. Please comment!
I wonder why do we return boolean for most of the file operations and also specify that exception will be thrown if error occurs. When the method will return "false" then? I think we should either return boolean or throw exception, not both. About listMultiple and getFileMultiple -- I like Martin's proposal in comment 2 for returning HostFileStatus for the failed queries.
Right, the inconsistency of returning boolean and throwing exception has been in RSE ever since I know it. Personally I like throwing exceptions much better because the status and problem indication can be much more verbose in this case. I would be OK with changing the method signatures to return void, but we need to be aware that this will likely break many clients. It would be a source breaking change though, which can be fixed VERY easily, so I'm in favor of doing it. Thoughts?
+1 for using exceptions and returning void in the IFileService This is a design flaw and we have a chance to fix it for RSE 3.0
(In reply to comment #7) > The doc now says for listMultiple(): > > If an error occurs during the retrieval of the contents of a folder, this > operation stops on that folder and a {@link SystemMessageException} is > thrown. Items retrieved before that folder will be returned. > > I wonder how that should work technically if an exception is thrown? The result > of the method is passed by return value, which is not available in case of an > exception. > > Is this a reason for actually changing the method specification, and having the > client pass in an empty List that the method populates with the IHostFile > elements to be retrieved? > > I think we can still do that, since the API is new in RSE 3.0 so it's not to be > considered a breaking change. Please comment! The javadoc is, of course, incorrect. I believe the way it should have read is that the array is not returned if there is an exception. You are right that if we were to change the signature to allow a partial result the we should accept a list argument. I think this might be a good thing, but I don't think we can make any statement about the cardinality of the returned items. We'll have to say something like: in the event of an exception being thrown the resulting set may have some items in it. These items will be correct but not all items may have been retrieved.
(In reply to comment #9) > Right, the inconsistency of returning boolean and throwing exception has been in > RSE ever since I know it. Personally I like throwing exceptions much better > because the status and problem indication can be much more verbose in this case. > > I would be OK with changing the method signatures to return void, but we need to > be aware that this will likely break many clients. It would be a source breaking > change though, which can be fixed VERY easily, so I'm in favor of doing it. > > Thoughts? +1
(In reply to comment #11) > make any statement about the cardinality of the returned items. We'll have to > say something like: in the event of an exception being thrown the resulting set > may have some items in it. These items will be correct but not all items may > have been retrieved. Sounds very good to me. I'm wondering whether we should also say something about the ordering, e.g. the order of items in the result list will match the order of folders to be queried. Then if user queried folders a,b,c,d and the result set contains [ a1, a2, a3, b1, c1 ] the user will know that a,b have been retrieved completely while the exception happened while retrieving c. So, the client will not need to try retrieving a,b again. Avoiding any statement about the ordering might make sense, because it would allow us to perform file system queries in parallel e.g. query a,b,c,d at the same time; but if we did that, any partial result will be completely unusable for the client and he'd have to perform sequential query.
I'm in the process of developing fixes to both the listMultiple and boolean return bits. Waiting for DaveM's vote on this prior to commit. The changes are relatively simple transformations.
I'm fine with making the changes. Only concern is that it's almost certain to break the code for existing RSE-based products since file queries are heavily used.
Yes, you're right and I know. But as a matter of fact, using the boolean return value for any kind of decisions in the client would have been invalid anyways, so it's probably better breaking them such that they can fix their code to react to the exception and only the exception. Guess that's your "GO" then, Dave.
The changes are not quite as simple as originally thought (always the case). DStoreFileService was returning false in many cases rather than throwing an Exception. This means that a SystemMessageException has to be fabricated and thrown in these cases. The good news is that it stands to simplifiy the code. Dave -- I'm going to need you to inspect this at some point.
Looks like dstore would have been buggy (violating the API contract). Good that we found this - thanks Rado.
(In reply to comment #18) > Looks like dstore would have been buggy (violating the API contract). Good that > we found this - thanks Rado. I've got a patch that changes cases where the dstore file service return false to instead throw an exception, however, I'm waiting on another patch (involving an API change) that includes this same file.
(In reply to comment #19) > (In reply to comment #18) > > Looks like dstore would have been buggy (violating the API contract). Good that > > we found this - thanks Rado. > > I've got a patch that changes cases where the dstore file service return false > to instead throw an exception, however, I'm waiting on another patch (involving > an API change) that includes this same file. > Good. Why don't you commit that when you get a chance (after testing)? I'll fold in the API changes after receiving it. That should make my work much easier.
(In reply to comment #20) > (In reply to comment #19) > > (In reply to comment #18) > > > Looks like dstore would have been buggy (violating the API contract). Good that > > > we found this - thanks Rado. > > > > I've got a patch that changes cases where the dstore file service return false > > to instead throw an exception, however, I'm waiting on another patch (involving > > an API change) that includes this same file. > > > Good. Why don't you commit that when you get a chance (after testing)? I'll > fold in the API changes after receiving it. That should make my work much > easier. The other patch is for bug 229610. Do you want me to commit that?
I've committed the change to DStoreFileService to throw exceptions instead of return false so hopefully this will be easier to proceed with now.
(In reply to comment #22) > I've committed the change to DStoreFileService to throw exceptions instead of > return false so hopefully this will be easier to proceed with now. Can the lower level DStore Service throw any SystemMessageException? - If yes, we should avoid wrappering these in a RemoteFileIOException in the DStoreFileService.
(In reply to comment #23) > (In reply to comment #22) > > I've committed the change to DStoreFileService to throw exceptions instead of > > return false so hopefully this will be easier to proceed with now. > Can the lower level DStore Service throw any SystemMessageException? - If yes, > we should avoid wrappering these in a RemoteFileIOException in the > DStoreFileService. RemoteFileIOException extends RemoteFileException which extends SystemMessageException.
No, I meant something different - If we do: try { lowerLevelDStore.performStuff(); } catch(Exception e) { throw new RemoteFileIOException(e); } and the lowerLevelDStore throws a SystemMessageException, then we will wrapper that SystemMessageException inside a RemoteFileIOException. Which might not be desirable, since the original DetailMessage will no longer be visible to the user. What I've been doing in SSH and FTP is this: try { lowerLevel.performStuff(); } catch(SystemMessageException e) { throw e; //dont wrapper message exceptions again } catch(Exception e) { throw new RemoteFileIOException(e); } The other issue is, that the DStoreFileService currently has compile errors in HEAD. Please get these fixed such that I can tag & release into an I-build.
(In reply to comment #25) > No, I meant something different - If we do: > try { > lowerLevelDStore.performStuff(); > } catch(Exception e) { > throw new RemoteFileIOException(e); > } > and the lowerLevelDStore throws a SystemMessageException, then we will wrapper > that SystemMessageException inside a RemoteFileIOException. Which might not be > desirable, since the original DetailMessage will no longer be visible to the > user. What I've been doing in SSH and FTP is this: > try { > lowerLevel.performStuff(); > } catch(SystemMessageException e) { > throw e; //dont wrapper message exceptions again > } catch(Exception e) { > throw new RemoteFileIOException(e); > } Oh I think I understand. You just mean that we should always have a: catch(SystemMessageException e) { throw e; } before any other caches, right? > The other issue is, that the DStoreFileService currently has compile errors in > HEAD. Please get these fixed such that I can tag & release into an I-build. Hmm, I'm not seeing the errors. Could you tell me where there show for you?
Only before catching an Exception that's in the hierarchy above SystemMessageException, and only if the lowerLevel stuff can throw SystemMessageException. I'm getting compile errors in DStoreFileService: makeSureLocalExists(File) - because it throws RemoteFileIOException but is not declared to throw anything setLastModified() - because there is one if() clause that falls through and does not return anything, neither boolean nor exception setReadOnly() - for the same reason I'm very surprised that you do not see these compile errors.
(In reply to comment #27) > Only before catching an Exception that's in the hierarchy above > SystemMessageException, and only if the lowerLevel stuff can throw > SystemMessageException. In most cases in this file, SystemMessageExceptions aren't thrown. > I'm getting compile errors in DStoreFileService: > makeSureLocalExists(File) > - because it throws RemoteFileIOException but is not declared to throw anything > setLastModified() > - because there is one if() clause that falls through and does not return > anything, neither boolean nor exception > setReadOnly() > - for the same reason > I'm very surprised that you do not see these compile errors. Very strange. I find them now in the editor, itself, but oddly, the problems view isn't showing these errors (may be a good time to reboot eclipse). I've updated the code to fix the errors.
I have updated IFileService to "return" void for the following methods upload (both flavors) uploadMultiple download downloadMultiple delete deleteBatch rename (both flavors) move copy copyBatch setLastModified setReadOnly The listMultiple methods have been changed to take a List argument to contain IHostFile objects and "return" void. This affected all the file service implementations and I've made the changes as best I could, but I don't own the services and may have made some mistakes. In some cases I had to invent messages to make SystemMessageExceptions out of and introduced new strings. I'm sure strings already exists for these conditions. Please inspect your code and make the appropriate changes as necessary. I've done sniff tests on local and SSH, and they appear to work. You will need to perform much more extensive testing.
Committing the changes I've made. Please inspect and make appropriate changes to your services using this bug report. Assigning this to Dave M as I now believe the documentation to be correct.
What's generally missing in SftpFileService, is that in case a timeout occurs trying to acquire the fDirChannelMutex, the methods now return as if they had been successfull -- but they are not. We'll need a new kind of SystemMessageException for Timeout trying to acquire remote channel or remote resource. Since we'll need the same kind of exception as well for FTP -- Dave, could you add a generic such exception class? Will likely require a new PII String in order to indicate the timeout to the user; or, just throw the same exception as had been thrown before when "false" was returned from the methods. Note that bug 226374 is already open for API extensions for new kinds of SystemMessageException to be re-used by the Services.
The other thing I notice, is that with the IFileService methods no longer returning a boolean, the IRemoteFileSubSystem methods should likely also no longer return a boolean?
(In reply to comment #32) > The other thing I notice, is that with the IFileService methods no longer > returning a boolean, the IRemoteFileSubSystem methods should likely also no > longer return a boolean? I noticed that as well. We should open a new bug report for that and fix it in RC1.
We still have a problem with the spec of IFileService#getFileMultiple -- it says that if an error occurs an exception is thrown and some items are returned which is incorrect. We should refactor it to something like: void getFileMultiple(String remoteParents[], String names[], List hostFiles, IProgressMonitor monitor) throws SystemMessageException; Other than this I think the APIs are much better now.
The compilation errors for WinCEFileService are fixed now.
What's left to do with this one now? Are we waiting for bug 226374 or can we close this one and deal with outstanding issues in another defect?
Did you review all dstore changes? - Ssh and FTP are buggy as per comment 27. Also, it looks like Rado found another API flaw as per comment 34. DaveD can you comment on this one? I don't want to wait for bug 226374 but rather try and mimic the old-style behavior wherever possible such that the refactorings due to this bug have as little impact as possible.
Is there any particular reason why we're returning query results via List instead of as an array like IHostFile[]? I would have expected the array to be safer.
(In reply to comment #38) > Is there any particular reason why we're returning query results via List > instead of as an array like IHostFile[]? I would have expected the array to be > safer. > In this case, the API states that results are appended to the list. It is up to the caller to either provide an empty list or a list in which the results may be safely appended. An array is not suitable in this case since it may not be extended. The size is unknown ahead of time. Returning an array is not suitable since an exception may be thrown during processing and we wish to produce partial results.
(In reply to comment #34) > We still have a problem with the spec of IFileService#getFileMultiple -- it > says that if an error occurs an exception is thrown and some items are returned > which is incorrect. We should refactor it to something like: > void getFileMultiple(String remoteParents[], String names[], List hostFiles, > IProgressMonitor monitor) throws SystemMessageException; > Other than this I think the APIs are much better now. > I will change the signature for this as well and document the change.
(In reply to comment #40) > (In reply to comment #34) > > We still have a problem with the spec of IFileService#getFileMultiple -- it > > says that if an error occurs an exception is thrown and some items are > returned > > which is incorrect. We should refactor it to something like: > > void getFileMultiple(String remoteParents[], String names[], List hostFiles, > > IProgressMonitor monitor) throws SystemMessageException; > > Other than this I think the APIs are much better now. > > > > I will change the signature for this as well and document the change. the getFileMultiple changes have been committed
When reviewing AbstractFileService, I came across two questions: (1) Currently, the AbstractFileService default implementations of the "multi" queries never check the progress monitor. That is, they rely on the "non-multi" queries being well-behaved in the sense that they throw an RemoteFileCancelledException when the user cancels the monitor. For WinCE, for instance, this exception is currently *not* thrown. Is it OK to expect this? Or should the iterators in AbstractFileService check the monitor themselves? And: Shouldn't there be SubProgressMonitor in order to provide proper progress reporting when iterating over the list? The "old" implementations would stop their for() loop because they checked the "result" return value on each iteration. Did we introduce any regression here? (2) What if the listMultiple() queries returned a List of IHostFile[] arrays, one array for each parent requested, rather than a flat list of all IHostFile objects? Would that simplify usage for the clients or make it more complicated? (3) I think that in FileServiceSubSystem.delete(), we need to catch Exceptions from the service and do the folderOrFile.markStale() in a finally{} clause even if an exception is thrown. Because we don't know whether exception is thrown before or after deleting the element, we must mark it stale under all circumstances. Note that deleteBatch() does it the other way round, and marks all files to be deleted stale before actually requesting deletion. Would this be a better approach for the normal delete() operation as well? Same for the move() operation. (4) The WinCE setLastModified() and setReadOnly() operations should probably throw a "not implemented" exception rather than silently doing nothing. Will we need a generic kind of exception that we can throw if we want silent non-execution without reporting to the user?? -- That would actually be the correct counterpart of returning false in the old API. I cannot remember whether we already introduced some "capabilities" kind of API where the subsystem can check whether a Service actually supports setLastModified() and setReadOnly() or not. (5) In LocalFileService, all the methods using an archive handler are currently buggy. Because the handler returns "false" in case of an error, and LocalFileService must throw an exception in that case. (6) for Sftp and FTP, I already mentioned the problem that boolean hasSucceeded == false must be translated into an exception (of unspecified type). All questions could be tracked in separate bugs if we find it worthwile.
(7) I think the Javadoc in IRemoteFileSubSystem is now partially incorrect, e.g. for deleteBatch() it doesn't indicate that deletion will stop at the first error -- it currently says that it tries to delete what it can, and then throws an exception. (8) The delete() method used to return false in case the item to be deleted did not exist. In deleteBatch(), returning false was not a reason to stop batch operation. This particular case shows that we need a special exception to indicate "silent non-execution" which is a particular use-case but not and exception.... ... I actually tend to request adding the "boolean" return value back for the special case of delete(), any thoughts on that?
(9) In IRemoteFileSubSystem, the setLastModified() and setReadOnly() methods are currently documented to silently return "false" if the file to change does not exist. Is this what we really want? Particularly, what do we want to do in the following situations: (9a) operation is not implemented in the Service --> Throw an exception, or silently do nothing? Consider the case in uploadResourcesFromWorkspace() when the "PRESERVETIMESTAMPS" preference is switched on. (9b) Item to work on did not exist on remote. Throw an exception or silently do nothing? Or should we revert to returning a boolean for these two cases?
For archive handlers (which do not throw exceptions currently) this is related to bug 199854 -- it's a shame that an error in the archive handler, even if it is something like out of disk space, is not properly reported. Should we really go and report an "Unknown error in archive handler" here in the FileService, or should we go and fix the archive handlers to throw exceptions as well? - Bug 199854 is currently marked "Future" but given that this is a breaking API change it might be worth fixing it now.
Fixed (1): [221211] Fix progress monitor and cancellation for multi operations
I think that [2] should just return a flat list as it does now. The hierarchy is implied by the host file objects. If we want a structured form, we can create a new method for that as an enhancement.
(In reply to comment #42) Fixed (3) [221211] Fix markStale() for delete() operation with exceptions Fixed (4) [221211] Throw SystemUnsupportedOperationException for WinCE setLastModified() and setReadOnly()
(In reply to comment #31) > What's generally missing in SftpFileService, is that in case a timeout occurs > trying to acquire the fDirChannelMutex, the methods now return as if they had > been successfull -- but they are not. Fixed this by throwing SystemLockTimeoutException() as per bug 226374; also fixed some other bugs from the migration, e.g. not checking rv==0 after Sftp runCOmmand(), or not properly doing FTP recursive folder deletion; Fix committed, [221211] Throw SystemLockTimeoutException in Sftp and FTP in case a lock can not be obtained
(In reply to comment #42) > (5) In LocalFileService, Because the handler returns "false" in case of an > error, and LocalFileService must throw an exception in that case. --> Fixed with bug 199854 > (6) for Sftp and FTP, I already mentioned the problem that boolean > hasSucceeded == false must be translated into an exception --> Fixed > (7) I think the Javadoc in IRemoteFileSubSystem is now partially incorrect, > e.g. for deleteBatch() it doesn't indicate that deletion will stop at the > first error --> Filed bug 230821 to track this > (8) ... I actually tend to request adding the "boolean" return value back for > the special case of delete(), any thoughts on that? --> Done, Fixed, delete() returns a well-defined boolean again; fixed some issue with not reporting exception in some cases on Local, [221211] IFileService API for batch operations IFileService.delete() returns boolean false for silent no-op
(In reply to comment #44) > (9) In IRemoteFileSubSystem, the setLastModified() and setReadOnly() methods --> Also tracking this in bug 230821 now. As far as I can tell, I have reviewed and fixed all potential issues. I'll mark this fixed for now and have the JUnit tests run on it.
(In reply to comment #43) > (8) The delete() method used to return false in case the item to be deleted > did not exist. In deleteBatch(), returning false was not a reason to stop > batch operation. This particular case shows that we need a special > exception > to indicate "silent non-execution" which is a particular use-case but not > and exception.... > ... I actually tend to request adding the "boolean" return value back for > the special case of delete(), any thoughts on that? I disagree that we need the boolean return. This may be an use-case for deleteBatch but it is indeed an exceptional situation for delete(). Some client has requested a delete operation -- it can fail for many reasons and one particular reason is that the specified file doesn't exist, I consider that an exceptional situation. I suggest to add another exception (e.g. SystemFileNotFoundException) and catch it in deleteBatch.
My point was, that in case of "delete" when the file was not actually there, the desired result is achieved -- file is not at the speicified location any more, so one could argue that this is not an exceptional situation. On the other hand you are right, the user (or program) knew about a file X and wanted to delete it, and suddenly it's not there any more -- this is an exceptional situation and should be reported, and I like the suggestion of a RemoteFileNotFoundException. In fact, the concept of not finding any kind of RSE Element not at the specified place is a more generic one, so I'd prefer not limiting this to files -- what about a SystemRemoteElementNotFoundException for this case? Could be re-used by Processes for instance, when we're about to kill a process and it's not there any more. Any other thoughts or comments?
