Community
Participate
Working Groups
As an extension of the API introduced through bug #177332, I'd like to see an API for registering a callback to know when the RSE model is fully restored. We need such a callback in IRSEPersistenceManager, but also need to rewrite the InitRSEJob such that 1.) Clients can register early-startup-calls to be performed when the RSE model is initialized for the first time; such client initialization handlers should be chained, to ensure that the final "everything complete" flag is only set once really all clients are complete. An extension point is likely needed for this. 2.) Clients can know when the RSEUIPlugin$InitRSEJob is done. 3.) It may be necessary to move the whole model initialization (which is currently done by RSEUIPlugin$InitRSEJob) from UI into Core, to ensure that headless applications also get a fully initialized RSE model as needed. Actually, the extension point for clients to contribute should probably have a flag to indicate whether intialization is - to be done in UI mode only or also in headless mode - to be done on first workspace creation only or whenever RSE starts Once the callback API is in place, the code for deferred initialization of the SystemView (bug #197025) should be changed to make use of the callbacks.
For (2) - wait until InitRSEJob is complete - clients can already use the following code with TM / RSE 2.0: Job[] jobs = Job.getJobManager().find(null); for(int i=0; i<jobs.length; i++) { if ("Initialize RSE".equals(jobs[i].getName())) { //$NON-NLS-1$ jobs[i].join(); break; } } We are now using this method in our unit tests (bug #198956), as well as SystemView#createPartControl (bug #197025). Clients are encouraged to do the same (bug #194802).
*** Bug 220320 has been marked as a duplicate of this bug. ***
Note that as per bug 220320, just doing Job.join() may not be sufficient in some cases where a plugin tries to wait for RSE Initialization extermely early. In this case, a JobChangeListener needs to be registered such that only after the Job changes from WAITING to RUNNING we join. Scheduling for 3.0M6 since this is new API.
1.) Clients can register early-startup-calls to be performed when the RSE model is initialized for the first time; such client initialization handlers should be chained, to ensure that the final "everything complete" flag is only set once really all clients are complete. An extension point is likely needed for this. I'm not sure I understand what this might be used for. It sounds like you want to register two kinds of listeners - ones that are declared in an extension point and are called at the end of initialization but prior to saying "init complete" and another set that gets called after "init complete" is set. This latter set would probably be registered by the prior set. Am I understanding this correctly? I suspect there should be rules about what kinds of work can be done in the prior set - e.g. no further jobs started.
No, I'm talking about two different kinds: (a) Consider the "Local" node. It's auto-created by default on an empty workspace when RSE is started the first time. Other contributors might also want to auto-create some connections but only the first time, and especially "lazily" i.e. only when the RSE is initialized by some user action. This is what you call the "first set": initialization contributors. I'd think that a contributor can do whatever it wants, provided that it tells RSE somehow when it's finished. (b) Initialization-complete-listeners. Kinds of clients that want to start their work only when they know for sure that the model has been initialized properly. For instance, one might want to enable an "Add Host" button only when it's known that all systemTypes have been initialized by their dynamic providers and the model has been read in completely.
Regarding (a) It seems that each "initializer" should provide its own indication of when it is done. This will allow it to structure itself any way it wishes. If the initializer is running synchronously with the init job then it will mark itself as complete before we begin the next initializer. If the initializer is running asynchronous work, it will not t be marked complete. Therefore at the end of the init job we will have to run polling loop to determine initializer completeness. I assume there should be a timeout mechanism for this, any suggestions for its design? So the mechanism looks something like this: // run the initializers for each initializer { initializer.run(); if (!initializer.isComplete()) { incompleteSet.add(initializer); } } // poll the initializers while (!incompleteSet.isEmpty()) for each initializer in the incompleteSet { if (initializer.isComplete()) remove it from the set // there are ways to do this safely } sleep for a bit if (timeout time as arrived) break } // call back to to the listeners for initialization complete. ...
I don't think it is a good idea to let initializers run in parallel. They potentially modify the RSE model, and could make it inconsistent when they try doing so at the same time. I'd prefer forcing initializers to run sequentially only. Next initializer in the chain will only be started when previous initializer has signaled completion. When the last initializer in the chain has signaled completion, all of RSE is considered complete. I also don't think that introducing a timeout is a good idea. What would a proper value be? - In our commercial product, we have seen initialization of Makefile Fragments taking up to the range of 10 minutes. The build system was not usable while these were generated, but it happened only the first time after installation, and it happened in a background job. There is no problem with initializer jobs taking long as long as they eventually complete. It's the fault of the initializer and not the fault of the framework if it's slow. So a product shipping plugins with slow initializers is responsible for the integration testing and ensuring that initializers eventually complete.
Good point. It simplifies the code a bit too. So the code looks something like this. We should of course handle exceptions and use a progress monitor so they can report progress. restore the model from its persistent form // run the initializers, these may obtain and supplement the model using API for each initializer { monitor = a subprogressmonitor of the init job's monitor try { initializer.run(monitor); while (!initializer.isComplete()) { wait for a bit } } catch (Throwable t) { log t } monitor.done(); // just in case the initializer didn't do this, multiple done calls are allowed } // call back to to the listeners indicating initialization complete.
I've implemented the initializer mechanism and placed it in a patch. I've also moved the InitRSEJob into core and used the initializers to create the Local connection as part of the UI initialization. Core will now initialize without it. There is a new method InitRSEJob.waitForCompletion() that will perform essentially the same incantation we've been telling folks to use for initialization. If users wish to be notified in a callback form they can use a JobChangeListener and monitor for the RUNNING->NONE transition. The API and the extension point are now fully documented. Previous API should work as described although the package of InitRSEJob has changed. A patch will be attached for a quick review before committing.
Created attachment 91472 [details] implemenation of initialization and notification for InitRSEJob implements a new waitForCompletion and allows initializers to be called as part of initializing RSE
Created attachment 91473 [details] mylyn/context/zip
API Summary org.eclipse.rse.core.InitRSEJob has been moved as an internal class in RSEUIPlugin to a top level class in org.eclipse.rse.core. This is a new class and is now publicly available API. New methods are static getInstance() - returns the job itself static waitForCompletion() - causes the executing thread to wait for completion of the singleton job isComplete() - returns a boolean indicating completion of this job (but not its completion status) The "run()" method, while public is not client API but for use by the platform's job manager. org.eclipse.rse.core.Messages not really public, just strings for InitRSEJob. new extension point org.eclipse.rse.core.modelInitializers So while this is new API, there are no breaking API changes.
Martin -- can you give this a quick look. I think it satisfies the intent of the request and gives Ankit what he desires as well. -- Dave
I don't think it's a good idea to make the InitRSEJob public as a whole. That's exposing an implementation detail that doesn't need to be exposed. I'd prefer having public API methods only for RSECorePlugin.isInitComplete(); RSECorePlugin.waitForInitCompletion() throws InterruptedException; and, optionally, RSECorePlugin.addInitCompleteListener(); Also, regarding the Messages, there is no such thing as "not really public". If public then we need to maintain it in the future. Otherwise, better move to internal or create a new class in internal.
We might also want to add API that allows clients to wait for certain phases of initialization to be complete. For instance, a client could want to wait till the basic model and SystemRegistry are initialized, but not be interested in additional initialization steps from extenders.
(In reply to comment #14) > I don't think it's a good idea to make the InitRSEJob public as a whole. That's > exposing an implementation detail that doesn't need to be exposed. > > I'd prefer having public API methods only for > RSECorePlugin.isInitComplete(); > RSECorePlugin.waitForInitCompletion() throws InterruptedException; > and, optionally, > RSECorePlugin.addInitCompleteListener(); > > Also, regarding the Messages, there is no such thing as "not really public". If > public then we need to maintain it in the future. Otherwise, better move to > internal or create a new class in internal. Fair enough. I see the point. Thanks.
(In reply to comment #15) > We might also want to add API that allows clients to wait for certain phases of > initialization to be complete. For instance, a client could want to wait till > the basic model and SystemRegistry are initialized, but not be interested in > additional initialization steps from extenders. This is something I'd rather hold for later. Without a specific use case I'm not sure we would guess the right points.
(In reply to comment #17) > This is something I'd rather hold for later. Without a specific use case I'm > not sure we would guess the right points. My point is that even if we don't support multiple phases from the beginning, we could design the API to account for those in the future: public boolean isInitComplete(int init_phase); public static final int INIT_PHASE_MODEL = 1; public static final int INIT_PHASE_ALL = 2; More int constants can be added to the API in a binary compatible manner later.
API Summary org.eclipse.rse.core.RSECorePlugin static waitForCompletion() - causes the executing thread to wait for completion of the initialization job static isComplete(int phase) - returns a boolean indicating completion of a particular phase of the initialization job static addInitListener(IRSEInitListener listener) - adds a listener to the set of listeners for init phase change events static removeInitListener(IRSEInitListener listener) - removes a listener from the set of listeners for init phase changes constants added for phases: static int INIT_ALL = 999 static int INIT_MODEL = 1 org.eclipse.rse.core.IRSEInitListener - defines the interface for listening to init phase changes This is new API, there are no breaking API changes.
The change looks great except for one thing: Because the RSEUIPlugin declares an InitRSE Extension, the RSEUIPlugin will be activated as soon as RSEcorePlugin is activated. I'm not sure if this is really what we want. Either we want to have the "Local" host added to the model even if UI is not loaded; then the initializer should be in RSE.core. Or, we want to wait with adding the "Local" host until UI is loaded - then it should not be declared as a modelInitializer extension, but a Job for adding the Local host should simply be scheduled when rse.ui initializes. I think that as a rule of thumb, model initializers should typically be non-UI because that's their main benefit -- they can initialize the model even before UI is activated. In fact they are very similar to the AbstractPreferenceInitializer added with Eclipse 3.0, see http://help.eclipse.org/help33/topic/org.eclipse.platform.doc.isv/reference/extension-points/org_eclipse_core_runtime_preferences.html We should perhaps review that code once again and ensure that ours is working similar. I'm not re-opening the bug though since the desired API has been added and behavior has not become worse compared to the previous situation. Feel free to file a new bug as clone if you don't think you can address it immediately.
Checking implementation again, I think that 1.) RSECorePlugin#waitForCompletion() should also have an "int phase" parameter 2.) InitRSEJob#isComplete needs to be volatile or protected by synchronized 3.) InitRSEJob#isModelComplete needs to be volatile or protected by synchronized 4.) InitRSEJob#waitForCompletion should not Thread.sleep() but install a Job Change Listener in a nested class and wait() on status change of that class -- I hate Polling, and 10 seconds is way too long IMHO 5.) I don't think an InterruptedException on the waitForCompletion should be logged as an error. It's normal behavior when users starts up Eclipse and chooses to exit Eclipse before everything gets a chance to get initialized. 6.) In InitRSEJob#run() you're setting isComplete == true even if the Job was canceled by the Monitor... I don't think this is a good idea! I think that when the Job is canceled, all waiting Threads should throw an InterruptedException. 7.) The initializers are missing a feature: Just like we do for the "Local" host, initializers should have a "run-once-per-workspace-only" capability in their extension. When that is set TRUE, the InitRSEJob should use a mark file in the workspace metadata to see if the initializer has ever been run before. If yes, it should not even instantiate the initializer --> saves instantiating plugins if not necessary. Somehow I also think you should instantiate/run each initializer in turn rather than first instantiating them all then running them all. 8.) notifyListeners: Why use ArrayList myListeners? A simple array would be good enough here: IRSEInitListener[] myListeners; synchronized (listeners) { myListeners = (IRSEInitListener[])listeners.toArray( new IRSEInitListener[listeners.size()]); } for (int i=0; i<myListeners.length; i++) { try { myListeners[i].phaseComplete(phase); } catch (RuntimeException e) { logger.logError(...); } } 9.) In getInstance() it's safer to do it like in ArchiveHandlerManager -- your lazy init with null check can actually lead to TWO instances which cannot happen in the other approach, and thanks to the Classloader the other approach is lazy as well. You don't need the initialize-on-demand holder class idiom here since you have no other static methods in InitRSEJob: http://www.theserverside.com/discussions/thread.tss?thread_id=35349 Reopening now because issues (2), (3), (6) are critical logic problems and issue (4) may pose unnecessary performance limitation.
And another one: 10) Why use a HashSet for the listeners? There is no need for random access to the listeners. HashSet needs a lot more memory than a simple ArrayList which should be good enough here.
I'm not sure I understand #6 of your list. The signature for run() does not allow throwing an InterruptedException and the contract calls for a cancel status to be returned in this case.
I thought something along these lines: boolean isCanceled = false; public void run() { /*...*/ if(status.isCanceled()) { isCanceled = true; anyWaiters.notifyAll(); } } public boolean isComplete(int phase) { /*...*/ if(isCanceled) return false; } public void waitForCompleteion() { while(!isComplete(phase) && !isCanceled) { /*...*/ } }
API Summary org.eclipse.rse.core.RSECorePlugin static waitForInitCompletion() - causes the executing thread to wait for completion of the initialization job static waitForInitCompletion(int phase) - causes the executing thread to wait for the completion of a particular phase of initialization static isInitComplete(int phase) - returns a boolean indicating completion of a particular phase of the initialization job static addInitListener(IRSEInitListener listener) - adds a listener to the set of listeners for init phase change events static removeInitListener(IRSEInitListener listener) - removes a listener from the set of listeners for init phase changes constants added for phases: static int INIT_ALL = 0 static int INIT_MODEL = 1 static int INIT_INITIALIZER = 2 org.eclipse.rse.core.IRSEInitListener - defines the interface for listening to init phase changes (changed slightly from the previous commit)
I've updated and reorganized the code according to Martin's suggestions. I've also removed the "isComplete" predicate on an initializer as it was serving no purpose other than controlling a poll with the potential for an initialization lockup for ill-behaved initializers. Initializers now simply run and return. If they choose do fork asynch work they may do so, but it is not up to the initialization framework to control synchronization with that work.
Ankit -- in the simple case you can just RSECorePlugin.waitForInitCompletion(); See the RSEUIInitJob for an example of its use in an asych context kicked off from the plugin startup code.
Looks very good now. The one problem I'm seeing now is that you call notifyListeners() from inside a synchronized block: public synchronized void RSEInitJob$Phase#done() This is an "open call" and very dangerous because the lock is being kept held on RSEInitJob$Phase so nobody else can enter it's corresponding monitor until the contributed, unknown method returns from notification. Deadlock could occur if the client being notified, as part of notification, switches to another thread (e.g. Display.syncExec()) and after that Thread switch queries completeness of a phase via RSECorePlugin.isInitializationComplete(). Since this is critical, I'm reopening once again to get it fixed.
You are absolutely correct. I should have caught that. Thanks. Now fixed.
