Comment 1 Nick Edgar 2005-05-10 17:30:20 EDT

John, I cc'ed you just FYI.  The early startup code runs as a job.  If there's a
cleaner way of doing this than an explicit check to
PlatformUI.isWorkbenchRunning(), please let me know.
Are jobs canceled on shutdown?

One option would be to wait for all early startups to complete before shutting
down, but it's probably acceptable just to short-circuit it.

Comment 2 John Arthorne 2005-05-10 17:42:16 EDT

Jobs are canceled by runtime when the runtime plugin shuts down, and a warning
is added to the log saying that a job was still running.  However, this is
generally too late to avoid ClassNotFoundExceptions. The two options are:

1) Cancel the early startup job during shutdown, and modify the early startup
job to check for cancelation before calling each extension (should probably
check for cancelation anyway to be a good citizen).

2) Within the early startup job, check PlatformUI.isRunning(), and/or
IWorkbench.isClosing() and bail out if shutting down.

Comment 3 Nick Edgar 2005-05-18 11:32:47 EDT

John, I don't currently hang onto the early startup job.  What would be the best
way to look it up in order to cancel it?

Comment 4 Nick Edgar 2005-05-18 11:36:19 EDT

Also, is it recommended to return a cancel status if the job is canceled?
e.g.

if (monitor.isCanceled()) {
  return Status.CANCEL_STATUS;
}

The Javodoc for Job.run isn't too clear on this.

Comment 5 Nick Edgar 2005-05-18 11:53:05 EDT

Also, it seems like we should wait until the early startup job completes (or is
canceled) before proceeding with workbench shutdown.  But IJobManager.join from
Workbench.shutdown() seems prone to deadlock (it gets called in the UI thread).

Comment 6 Nick Edgar 2005-05-18 11:54:02 EDT

I'm not going to put the join in for now, since otherwise we'll hang if one of
the early startup extensions does a syncExec.

Comment 7 John Arthorne 2005-05-18 11:55:27 EDT

To cancel a job, you need either the job object, or it needs to belong to a job
family.  If the job has a distinct family, it can be canceled using
IJobManager#cancel(family).  

Yes, returning CANCEL status from the job is the correct thing to do. In
practice this only really matters if there are job listeners that check the
job's result status.  The result is not interpreted in any special way by the
platform.  I have updated the javadoc of Job#run to reflect this.  (Note:
throwing OperationCanceledException will have the same effect as returning
Status.CANCEL_STATUS).

Comment 8 Nick Edgar 2005-05-18 12:25:44 EDT

Ah, using cancel(family) is nicer than what I had:

Job[] jobs = Platform.getJobManager().find(EARLY_STARTUP_FAMILY);
// There should be at most one.  If more than one, it should -definitely- be
canceled!
for (int i = 0; i < jobs.length; i++) {
	jobs[i].cancel();
}

Comment 9 Nick Edgar 2005-05-18 14:41:21 EDT

Fixed.

Comment 10 Nick Edgar 2005-05-27 15:50:02 EDT

The ClassNotFoundExceptions can still occur, since even though we check
isCanceled before running each startup extension, the workbench can still
shutdown in the middle of starting a single extension.

Need to block for the current extension to finish before shutting down.

Comment 11 Nick Edgar 2005-05-27 15:56:06 EDT

The test in WOrkbench.startPlugins for PlatformUI.isWorkbenchRunning() should
just call this.isRunning(), or it could be get a false positive if another
workbench has started.

Comment 12 Nick Edgar 2005-05-27 16:09:47 EDT

EarlyStartupRunnable should not do its own logging in handleException either, or
this can result in the following.  Should leave the logging up to Platform.run.

org.eclipse.core.internal.runtime.AssertionFailedException: assertion failed:
The application has not been initialized.
	at org.eclipse.core.internal.runtime.Assert.isTrue(Assert.java:107)
	at
org.eclipse.core.internal.runtime.InternalPlatform.assertInitialized(InternalPlatform.java:218)
	at
org.eclipse.core.internal.runtime.InternalPlatform.log(InternalPlatform.java:834)
	at org.eclipse.core.internal.runtime.Log.log(Log.java:56)
	at org.eclipse.ui.internal.WorkbenchPlugin.log(WorkbenchPlugin.java:664)
	at
org.eclipse.ui.internal.EarlyStartupRunnable.handleException(EarlyStartupRunnable.java:80)
	at
org.eclipse.core.internal.runtime.InternalPlatform.handleException(InternalPlatform.java:709)
	at
org.eclipse.core.internal.runtime.InternalPlatform.run(InternalPlatform.java:1042)
	at org.eclipse.core.runtime.Platform.run(Platform.java:775)
	at org.eclipse.ui.internal.Workbench$19.run(Workbench.java:1584)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java)

Comment 13 John Arthorne 2005-06-01 11:58:49 EDT

> Need to block for the current extension to finish before shutting down.

I wouldn't suggest joining the early startup job on shutdown because of the
deadlock risk, at least for 3.1.  There may be unexpected deadlocks if the early
startup thread is trying to access something in the UI thread while the UI
thread is waiting for it to complete, for example.  It's the kind of change that
should run for a month or two to flush out unexpected side-effects. A
ClassNotFoundException in the log file isn't as bad as a deadlock that prevents
save and shutdown from running...

Comment 14 Nick Edgar 2005-06-01 13:16:20 EDT

My short term memory seems to be shot.  I said the same thing in comment 5.

Comment 15 Nick Edgar 2005-06-06 22:22:19 EDT

John, what do you think of the idea of polling for 10 seconds or so to let the
last early startup extension finish.

E.g. in Workbench.cancelEarlyStartup(), replace:
	Platform.getJobManager().cancel(EARLY_STARTUP_FAMILY);

with:

	Platform.getJobManager().cancel(EARLY_STARTUP_FAMILY);
	long giveUpTime = System.currentTimeMillis() + 10000; // wait 10 seconds max
	while (Platform.getJobManager().find(EARLY_STARTUP_FAMILY).length > 0 &&
System.currentTimeMillis() < giveUpTime) {
		try {
			Thread.sleep(100);
		} catch (InterruptedException e) {
			break;
		}
	}

This way, even if an early startup item is blocked on the UI, it won't deadlock.
(Even before this change, it would not have been able to make progress because
at this point we're not spinning the event loop anymore).

We could also make the delay a hidden preference, in case an app needs to tailor
the value.

Comment 16 John Arthorne 2005-06-07 17:08:35 EDT

Is this still causing problems for the tests?  It seems like a complex solution
for what seems to be a rare corner case...  An exception in the log file for
this case doesn't seem much worse than a ten second delay, if it is likely to be
rare.  

I suppose if it's still causing problems for automated tests a short delay would
be ok, perhaps followed by an explicit warning in the log that an early startup
extension was still running at the time of shutdown.

Comment 17 Nick Edgar 2005-06-07 21:33:44 EDT

It's not just the tests I'm concerned about.  I'm more concerned about a real
startup extension that is long-running, and may leave the world in an
inconsistent state if the workbench and runtime shut down before it has finished.

There is a concern here that waiting will affect the performance tests though.

Comment 18 Michael Van Meekeren 2005-06-14 15:57:05 EDT

defering to 3.2 as this is not fully fixed yet.

Comment 19 Nick Edgar 2006-01-11 11:48:27 EST

Nothing planned for 3.2.  It hasn't been an issue in 3.1 or 3.2, AFAIK.

Comment 20 Nick Edgar 2006-01-11 11:49:03 EST

Actually, I'm going to mark this as fixed in 3.1.
Please reopen or file new bug reports if you see this reoccur.