RE: Thread/VMThread proposal

[Jeroen Frijters]

Hi David,

Thanks for taking the time to comment!

David Holmes wrote:
> - Thread.VMThread and Thread.group must be volatile as they are
> accessed by multiple threads without synchronization via isAlive() -
> or else ensure everything that reads them only does so while holding
> the thread's lock. See end comment

You are absolutely right. I'm hanging my head in shame, I really do know
better than that ;-)

> - VMThread.run allows exceptions to escape - which may not be a good
> thing :) Any exceptions from group.uncaughtException should be
> "swallowed". Any other exceptions would indicate a VM error and should
> probably be logged. If you are assuming this is always called from C
> code then the exceptions would be ignored anyway, but it may not be
> called from C code.

I added a try/catch(Throwable) around the call the uncaughtException.

> - You may need more lifecycle management in VMThread. For example, a
> thread that has stop() called on it before it is started is
> "stillborn".

The docs do say that, but my tests suggest that Sun just ignores stop()
when called on an unstarted thread. Should we implement what the docs
say, or emulate the Sun implementation? In most cases (including this
one), my preference is to do what Sun does.

> - VMThread.join() should be implemented using wait/notify not using a
> polling loop with sleep. Your VMThread.run would then do the notifyAll
> upon termination.

IMHO, this is too heavyweight for the default VMThread. I expect most
VMs to supply their own version of VMThread which has a better
implementation of join (based on some native join-like mechanism). I
agree the default is lame, but I don't think putting time into improving
it is worthwhile if no-one is going to use it. Of course, by this
argument it should probably be taken out entirely and made native.

> - having VMThread.currentThread create a Thread if the current
> VMThread is "unbound" seems inappropriate. currentThread should be a
> fast operation and you don't want even a null check in there. Further,
> the null condition would presumably only occur during bootstrapping,
> which implies a better interface is needed to set up the "primordial"
> Thread.

Unfortunately, I think this is required, because native threads can call
into Java code. Native threads don't have a corresponding Java thread
until Thread.currentThread is called (but I don't think this is
specified anywhere). This is an area that needs more investigation (and
documentation), because it is also conceivable that a VM chooses to
create the Java thread on the JNI boundary.

> - VMThread.getPriority may also need to be "native" otherwise you may
> have problems with setting the priority field atomically with respect
> to changing the actual runtime priority of the thread. As it stands,
> Thread.setPriority changes the priority field and then completely
> indepedently sets the VMThread priority - allowing the two to be
> different to an external observer. While normal Java doesn't guarantee
> the observability of this, a VM that does implement strict priority
> scheduling would allow this to be observed. You also need to ensure
> any changes in priority whilst a thread is in the process of starting
> are reflected in the actual priority of the started thread.

Again, I expect the VM implementer to provide a custom VMThread in this
case.

> - in general you need more synchronization to ensure atomicity of
> actions, and in particular to ensure a thread does not terminate
> whilst a method is being invoked upon it. Sequences such as:
>    VMThread t = this.vmThread;
>    if (t != null) t.XXX();
> can still throw NullPointerException if the thread terminates between
> the null check and the invocation.

No they can't. t is either null or it isn't, it doesn't change. Note
that I do assume that the native methods do the right thing, in case the
thread has already terminated.

> The simplest solution to make the
> vast majority of Thread methods synchronized (this is only a
> problematic solution if you are concerned about priority inversion
> :) ).

I went through the code again and looked at the memory model issues. I
made the setters in Thread synchronized and the mutable fields volatile.
Maybe I'm trying to be too clever (that often happens when people are
trying to "optimize" synchronization), but I really do understand the
Java memory model (and how flawed it is) and I want to get it right and
not prevent VMs with a stronger memory model from having a more
optimized version of VMThread.

I fixed a few other bugs (still haven't compiled it, so there will be
others) and attached the new version.

Regards,
Jeroen

VMThread.java
Description: VMThread.java

Thread.java
Description: Thread.java