views:

203

answers:

3

Given the J5+ memory model (JSR-133) is the following code thread-safe and permissible?

And if it's safe, is it acceptable in some situations?

public final class BackgroundProcessor
extends Object
implements Runnable
{

public BackgroundProcessor(...) {
    super();

    ...

    new Thread(this).start();
    }

public void run() {
    ...
    }
}

As I read the new JMM spec, initiating a thread creates a happens-before relationship with anything the initiated thread does.

Assume that the object has private member variables set in the constructor and used in run().

And the class is marked final to prevent sub-classing surprises.

Note: There is a similar question here, but this has a different angle: http://stackoverflow.com/questions/84285/calling-thread-start-within-its-own-constructor

A: 
  1. Your class should probably implements Runnable

  2. Yes, your code is OK

  3. It's not nessecary to extend Object. I'm sure that you know that Object is extended implicitly by all classes. I don't know where this popular practice to extend object explicitly comes from but it's a bad style.

Roman
Re 2. What?????
Tom Hawtin - tackline
@Roman: (1) Yes - oops; (2) I think so; (3) Bad style only IYO which you are entitled to (and an opinion I am sure many would share - I don't but my programming style is not the subject of this post).
Software Monkey
+3  A: 

JLS: Threads and Locks says

Two actions can be ordered by a happens-before relationship. If one action happens-before another, then the first is visible to and ordered before the second.

If we have two actions x and y, we write hb(x, y) to indicate that x happens-before y.

  • If x and y are actions of the same thread and x comes before y in program order, then hb(x, y).

and

A call to start() on a thread happens-before any actions in the started thread.

so we can conclude that all actions in the constructor before the call to Thread.start() happen before all actions in the thread that was started. If Thread.start() is after all writes to the object's fields, then the started thread will see all writes to the object's fields. If there are no other writes to the fields (that another thread will read), then the code is thread-safe.

Esko Luontola
Yeah, I read it the same way, but it seems to violate the admonition that one should not let a reference to "this" escape during construction (in the handful of case where I have done this, I think it's OK, and I don't do it often). In this case I am looking for confirmation of threading correctness, not good or bad style - the former is a high priority to fix, the latter is not.
Software Monkey
I think that the fact you declare the class final means that this will work. However, if you (or anyone else) ever removes that final declaration, you risk unsafe publishing of your objects. I'd refactor as a matter of principle, rather than of necessity.
Bill Michell
+1  A: 

Well, unless you particularly want to disagree with Brian Goetz on a Java concurrency issue, I'd assume it's wrong. Although his exact words are "is best not to...", I'd still heed his advice. From JCIP, p. 42:

"A common mistake that can let the 'this' reference escape during construction is to start a thread from a constructor [...] There's nothing wrong with creating a thread in a constructor, but it is best not to start the thread immediately. Instead, expose a start or initialzie method..."

Update: just to elaborate on why this is a problem. Although there's guaranteed to be a memory barrier between the call to Thread.start() and that thread's run() method actually beginning to execute, there's no such barrier between what happens during the constructor after the call to Thread.start(). So if some other variables are still to be set, or if the JVM carries out some "have-just-constructed-object" housekeeping, neither of these are guaranteed to be seen by the other thread: i.e. the object could be seen by the other thread in an incomplete state.

Incidentally, aside from whether or not it actually breaks the JMM, it also just feels a bit like an "odd thing to do" in a constructor.

Neil Coffey
Yes but why is it bad? And if it was broken I would expect wording like "you must not start a thread in your constructor", rather than "it's best not to...". Per Esko's answer, I tend to think the JMM spec defines a thread started at the end of a constructor as thread-safe. Also, is that quote WRT the revised JMM or the old one?
Software Monkey
The question as to whether Goetz is referring to the new or old JMM is important because one of the things that JSR-133 added was to make Thread.start() and Thread.join() explicit happens-before boundaries.
Software Monkey
Definitely the new memory model-- that's really the point of the book. Thread.start() does indeed constitute a happens-before boundary, but only in the sense that code before the Thread.start() call (i.e. not necessarily storing ALL of the object state set during the constructor) happens before the thread's run() method.
Neil Coffey
@Niel: There is no code *after* the thread start, and my question is specific to the example code with the thread started as the last thing in the constructor (BTW: I agree that it's odd to do, but it does make sense in the few places I have done it).
Software Monkey
Well, just be aware you're creating a ticking timebomb. The JVM you're running in may do some 'object comitting' process (e.g. comitting the values of final fields) when the constructor completes; I'd at least put a comment to alert a future maintainer of your code to accidentally adding such a field, and I'd test veeery carefully when you upgrade your VM...
Neil Coffey