Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 166233 - Support for StartupMonitor
Summary: Support for StartupMonitor
Status: RESOLVED FIXED
Alias: None
Product: Equinox
Classification: Eclipse Project
Component: Framework (show other bugs)
Version: unspecified   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.3 M4   Edit
Assignee: Thomas Watson CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 154088
  Show dependency tree
 
Reported: 2006-11-29 13:11 EST by Andrew Niefer CLA
Modified: 2006-12-04 17:43 EST (History)
3 users (show)

See Also:


Attachments
Initial Proposal (7.02 KB, patch)
2006-11-29 13:12 EST, Andrew Niefer CLA
no flags Details | Diff
Version 2 (9.03 KB, patch)
2006-12-01 12:08 EST, Andrew Niefer CLA
no flags Details | Diff
Version 3 (8.61 KB, patch)
2006-12-01 14:44 EST, Andrew Niefer CLA
no flags Details | Diff
Vesion 4 (8.90 KB, patch)
2006-12-01 14:59 EST, Thomas Watson CLA
no flags Details | Diff
patch (11.55 KB, patch)
2006-12-04 17:39 EST, Thomas Watson CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Niefer CLA 2006-11-29 13:11:06 EST
As part of the plan item to improve the launcher (bug 154088, see the wiki http://wiki.eclipse.org/index.php/Equinox_Launcher), we now require opportunity to periodically update a splash screen during startup.

The proposal for this is a startup monitor that runs while the main thread is waiting for the start level to finish incrementing.  

A default monitor is provided via the startup jar that calls back to native code to paint the splash.  Bundles can register a startup monitor as a service and as soon as a monitor is detected it is used instead of the default.

See the attached patch.  Some open questions are:
1) Which package should the StartupMonitor go in.  In the patch, it is in org.eclipse.core.runtime.adaptor which is marked as x-friends with core.runtime.

2) Does the StartupMonitor interface need anything more than the update() method?

3) Are there more locations other than setStartLevel where this should be done?  Perhaps refreshPackages().
Comment 1 Andrew Niefer CLA 2006-11-29 13:12:58 EST
Created attachment 54729 [details]
Initial Proposal
Comment 2 Andrew Niefer CLA 2006-11-29 13:16:03 EST
If possible we should consider this for M4
Comment 3 Jeff McAffer CLA 2006-11-29 23:41:38 EST
You want to put this in for M4?  will this work without the new launcher?

Some comments on the patch
- needs to be formatted with the Equinox formatter settings
- StartupMonitor is an interface.  Should it be IStartupMonitor?  I remember we had some whacky logic saying that some/all of the stuff in the OSGi bundle does not have I's.  If that's what we are doing, ok.  Just confirming.
- the new code in EclipseStarter will leak the FrameworkListener and trackers if an exception occurs in the loop (either in the user code or through an Interrupt)
- Since in effect we are calling listeners, we should probably protect the call site from misbehaved code using something equivalent to the SafeRunnable try/catch sequences.
- 100ms might be pretty long.  Clearly there is a trade off here but hopefully one run of the loop should not take too long...

- Stepping back, we need to consult with people like Kim and others to see if the "polling" approach will work best or whether we should simply delegate to the first (whatever) StartupMonitor that shows up.  The former is more general and more pluggable.  The latter may give a smoother experience.  Also, the nature of this code should sync up with however the UI workbench is going to drive the splash refresh/animation while the workbench is being started.  That is, when are they going to call who with what information?
Comment 4 Andrew Niefer CLA 2006-12-01 12:08:08 EST
Created attachment 54902 [details]
Version 2

Patch updated for Jeff's Comments:

> You want to put this in for M4?  will this work without the new launcher?
Yes, this will work without the new launcher and startup.jar.  In the initial patch, the DefaultStartupMonitor.update was a no-op in this case.  In the new patch, the defaultStartupMonitor will actually be null and won't be called.

> StartupMonitor is an interface.  Should it be IStartupMonitor?
I named it without the "I" because I didn't see any other "I" interfaces in the  osgi bundle.

> the new code in EclipseStarter will leak ... if an exception occurs
Wrapped the calls with a try catch, and added a finally to clean up the trackers (which may actually be redundant).

> 100ms might be pretty long. 
I removed the sleep and added a 50ms delay to the semaphore.acquire.  This way if the second thread finishes in that time we don't have that 1 extra iteration of the loop.

> Stepping back, we need to consult with people like Kim and others to see if
> the "polling" approach will work best or whether we should simply delegate to
> the first (whatever) StartupMonitor that shows up.

I talked to Kim and she prefers the polling method for the splash screen.  This also seems to be the method that will be used once the workbench takes over.

I like the polling method because it reinforces the idea that we are running in spare cycles and that these update calls should not take a long time.  The monitor would simply process any outstanding events and return instead of waiting for additional events.  It also removes the need to notify the monitors that it is time to return when the secondary thread is finished.

This patch also includes calls to the monitors during refreshPackages.  This case is slightly different in that we don't need to check for new services each iteration.
Comment 5 Thomas Watson CLA 2006-12-01 14:19:33 EST
I have not looked at the new patch, but one thing I noticed in the first patch was the splash progress got disabled.  This is because we no longer register the OutputStream service associated with the.  Was that intended?  Should you still be look for a getOutputStream method just in case?
Comment 6 Andrew Niefer CLA 2006-12-01 14:44:29 EST
Created attachment 54919 [details]
Version 3

Yes, you are right.  The new launcher won't have an output stream for progress, but we should keep it so that we can still work the same when the old launcher/startup.jar is used.
Comment 7 Thomas Watson CLA 2006-12-01 14:59:27 EST
Created attachment 54922 [details]
Vesion 4

This patch still publishes the OutputStream service if the getOutputStream method exists on the endSplashHandler.  This allows the existing splash progress to work.

I'm not sure where the new StartupMonitor interface should go but I'm pretty sure it should not go in org.eclipse.core.runtime.adaptor.  This patch moves the interface into the org.eclipse.osgi.service.runnable package for now.
Comment 8 Thomas Watson CLA 2006-12-04 17:39:01 EST
Created attachment 55002 [details]
patch

I'm going to release this patch for the next I-Build.

In discussions with Andrew we decided to keep the the monitoring of StartupMonitor services simple for now.  This patch just uses the highest ranking StartupMonitor service and registers the default monitor at the lowest possible ranking.  This will allow a new monitor to be registered and used later in the startup sequence.

I also fixed some bugs where we were not unregistering services on shutdown.
Comment 9 Thomas Watson CLA 2006-12-04 17:43:59 EST
patch released for the next I-Build.