Community
Participate
Working Groups
Any long running ISubSystem methods, which executes only if called from a non-display thread, must provide the possiblity to callback the invoking client. Basically this are all long running operations which should not block the UI, but at least methods like connect/disconnect and initialize/uninitialize. Furthermore it would be useful to have getChildren() and hasChildren() asynchronous as the query of these information might take a real long time.
Updating description (replacing the original one!): Certain operations on sub systems (or other RSE components) can be quite long running as communication channels needs to be established with the target and data might be exchanged. If the communication channel is narrow-band channel, like serial line, the operation execution time is not predictable. These operations should never block neither RSE nor the Eclipse platform because of the enforcing of synchronous API patterns. For such operations, sub systems (or other RSE components) should provide asynchronous API allowing to initiate an operation and notifying the initiator via callbacks or events about the end, the status and the result of the initiated asynchronous operation.
There is existing bug 142478 tracking the request to perform all long-running operations in background. We have already made a lot of progress here, putting most operations involving remote connections or data transfer in background jobs. Few operations currently remain synchronous but will be fixed soon (e.g. the Rename operation). Until now, we've been following the strategy to use Eclipse Jobs for background transfer. Is there a particular reason why you would prefer asynchronous calls and callbacks? Note that these would not necessarily be displayed in the Progress view and thus not easily be cancelable. Let's look at each of your items in detail: * ISubSystem.getChildren(), hasChildren() - The children of an ISubSystem are typically filters, which can be found and displayed synchronously. - If a subsystem does not use filters, it is free to synchronously return a (pending) message and update that one as soon as a job to retrieve data is finished. So I agree a callback would be helpful in this case. However, since the case of not using filters is not that common I think that's lower priority. * ISubSystem.connect(), disconnect() - These are currently called on the dispatch thread, and they typically schedule a job for connecting which is checked in a nested dispatch loop through SubSystem.scheduleJob() line 2111 ff. - I agree that the nested dispatch loop should be replaced by a real job with a callback to the client once the operation is finished. This is also covered by bug 160353. * ISubSystem.initialize(), uninitialize() - Javadoc says that these are called *after connect is finished*. These are meant for internal cleanup. I think it's OK to have them synchronous, even more since they do not have a return value. - In case a subsystem needs to perform long-running operations here, it is free to use an internal locking mechanism (e.g. an ISchedulingRule) to ensure that other queries are pended until internal cleanup is finished. So, in my opinion the connect()/disconnect() issue is the biggest one since it involves a nested event loop. Adding a dependency to bug 160353. The getChildren() issue seems less important but should also be addressed. For initialize()/uninitialize() I see no need to act.
>Until now, we've been following the strategy to use Eclipse Jobs for background >transfer. Is there a particular reason why you would prefer asynchronous calls >and callbacks? Note that these would not necessarily be displayed in the >Progress view and thus not easily be cancelable. Using jobs is not mutual exclusive to have asynchronous APIs. Asynchronous operations can have progress bars and can be cancelable without a problem. The Eclipse job model itself is offering the possibility of asynchronous jobs (even if this possibility is not sufficient enought to do asynchronous target communication). Is there is a particular reason ...? Yes, there is. Target communication has in a lot of case a more asynchronous nature than a synchronous one. It does not matter if the operation is a download, unload, run, query, connect, disconnect or whatever over ftp, ssh, dstore, serial line or whatever. Basically, for most of the operations which executes on the target, the steps to be taken - Getting the necessary data from the user - Initiating the operation including putting the operation in background and having a cancelable progress bar - React on the finished operation by probably showing a finished dialog or processing the data and run another query as reaction or whatever. There is no real need for step 2 and step 3 to be synchronous or even to be executed within the same thread context. These are our main reasons for strongly prefering asynchronous APIs over synchronous ones: - Synchronous behaviour can always be implemented using asynchronous APIs. The way around is quite more problematic. In our commercial product, the whole target communication is asynchronous. Mapping this to a synchronous API means a big step back and a lot of pain to us. - Do not waste CPU cycles by looping just to wait for the end of a operation - Improve overall UI usability by decreasing the possible blocking points in the API and by letting Eclipse decide what to do within the time the operation is executed at the target. - Having synchronous APIs for basically asynchronous operations tends to lead to have a lot of nested dispatch loops running in order to keep the UI running. It means as well that there are tendious more thread hanging around wasting CPU cycles and resources by just looping because of having to wait for results because of the synchronous API contract. Having more threads running means tendious more possibilities for dead locks to happen.
A lot of this stuff has already been addressed. Connection/disconnect run in jobs now that we don't wait for. Queries are run asynchronously via jobs. I think that main thing that remains to be addressed is the issue of having callbacks in response to the completion of asynchronous jobs.
Changed the description as indicated by comment #4. Re-instated the connection to the corresponding plan item.
I think we need callbacks by M6.
- The callback should be capable of returning a org.eclipse.core.runtime.IStatus for unified error, warnings, cancelation or other information reporting - The callback should be capable of passing on a optional result object - Callback thread semantic needs to be clear (guaranteed single-thread vs. thread-pool) It would be helpful if we could base the callbacks on Java 5.
Can you come up with a proposal for what the Callback should look like? Are you thinking about something along public interface IRSECallback { public void done(IStatus status, Object result); } being used like connect(new IRSECallback() { public void done(IStatus status, Object result) { handleConnectDone(status, result); } }); can you explain why you think Java5 would help with the callbacks. My first approach would be to always do callbacks through Display.asyncExec() though this may be problematic for headless applications. BTW, Java7 and closures may help make the syntax more readable in some distant future, see http://gafter.blogspot.com/
The proposal is quite exactly what I had in mind, except the use of Java 5. If using Java 5, a) we can gain type safety for the callback result by defining it as generic: public interface IRSECallback<V> { public void done(IStatus, V result); } That leaves more flexibility to the API declarations, which are using the callback, to exactly declare which type the to expect result object will have. I does avoid subclassing the interface again and again in different places just to bring in type safety with the possibilities of Java 1.4. b) we exactly _avoid_ using the SWT display thread for dispatching the callbacks as the Java 5 java.util.concurrent package is giving as all necessary to safely dispatching it in it's own dispatch thread. - b.1) The SWT display thread is for synchronizing and dispatching UI events. A asynchronous data query or even a connect is should not block the UI from being responsive. We shouldn't make the mistake to mix UI and non-UI needs just because we are afraid of using Java 5. The Eclipse UI needs to be as responsive as possible even if we would have to dispatch a lot of callbacks. - b.2) It simplifies the coding pattern if using callbacks. If dispatching via the SWT display thread, each and every callback implementer would have to think about and take care of doing as less non-UI work as possible within the SWT display thread by it's own. Typically, the callback will be processed in the thread it had been invoked. If this implies heavy data analysis in example, we do block the SWT display thread unnecessarly. It it a much better, much easier and much more Eclipse like pattern to dispatch the callback in it's own thread (or thread pool) and schedule explizit UI work from the callbacks via the built in Display.asyncExec or Display.syncExec. It is much easier to get clean code that way :).
(In reply to comment #9) We cannot use Java5 for RSE 2.0 because our project plan promised Java1.4. If we do the API with "Object" now, we can easily migrate to Java5 generics in the future (while we could not do it the other way round). Without Java5 util.concurrent, I propose sending the callback in the original caller's thread, which will typically be a background thread from a Job. See also bug 176603.
The callback and APIs now exist as follows: public interface IRSECallback { public void done(IStatus status, Object result); } and in ISubSystem: /** * Connect to the remote system on a background job. The callback is * called when the connect is complete. * * @param callback to call after connect is complete * @throws Exception */ public void connect(IRSECallback callback) throws Exception; /** * Connect to the remote system on a background job. The callback is * called when the connect is complete. * * @param forcePrompt forces the prompt dialog even if the password is in mem * @param callback to call after connect is complete * @throws Exception */ public void connect(boolean forcePrompt, IRSECallback callback) throws Exception; ConnectJob in SubSystem calls the callback after connect is done. The connect() and connect(boolean) methods of ISubSystem have been marked as deprecated but are still used. I'd like to change code in M7 to use the new APIs. If we're okay with the described APIs, then at least that part is done for M6.
moving to m7 now that the api is there.
Dave, the API looks good to me :)
When deferred queries are on, getChildren() should also have asynchronous callbacks in order to support recursive expansion of the system view over multiple levels easily (bug 176461).
Marking as fixed since the API is there. Moving to the new API for connect will be handled in bug #186363. The getChildren() thing will work without API change via the ContextObject, and will be tracked via bug #176461.
[target cleanup] 2.0 M7 was the original target milestone for this bug
