This simpler form, with the interruption logic corrected, seems fine to me.
David
On 30/01/2014 12:57 PM, Stuart Marks wrote:
Hi all, wow, lots of comments on this. Let me see if I can tackle them
in one message.
Quick aside before I get to the issues: my priorities for this code are
correctness and maintainability, possibly at the expense of performance.
In other words I'm willing to make the code more complex so that it's
correct, but I'm less willing to make it more complex in order to make
it go faster.
(Tristan, David) Making 'initialized' be volatile. As things stand, as
David has pointed out (thanks!) it's not necessary for it to be
volatile. There are other circumstances (see below) where it would be
necessary to make it volatile, though.
(Alan, Paul) We could convert to double-checked locking and avoid a
synchronization in the common case, paying only a volatile read.
Something like,
volatile boolean initialized = false;
...
private void awaitInitialized() {
if (!initialized) {
synchronized (this) {
try {
while (!initialized) {
wait();
} catch (InterruptedException ie) {
Thread.currentThread().interrupt();
}
}
}
}
I *think* that's right. (Is it?) I don't know whether this performs any
better, or if it does, whether it's worth the additional complexity. I
had to squint at this for a while to convince myself it's correct.
I am fairly sure this is not a performance-critical area of code.
(Famous last words, I know.) The other threads that can be active here
are handling remote calls, so they're already doing network I/O,
unmarshalling, and dispatching to the RMI thread pool. Compared to this,
the incremental cost of a synchronization block seems inconsequential. I
don't have much intuition about how much we'd save by substituting a
volatile read for a full synchronization in the common case, but given
that this is within the context of a fairly expensive operation, it
doesn't seem like it's worth it to me.
(Alan, Paul) Calling awaitInitialized isn't strictly necessary anywhere
except for the equals(NAME) case of lookup(). Yes, that's right. I added
it everywhere because of a possibly misguided sense of completeness and
consistency. One could essentially redefine awaitInitialized() to
protect just the systemStub field, not the "entire" object, whose only
state is that field anyway. Also, see below regarding renaming this method.
(Alan) Use systemStub == null as the condition instead of !initialized.
I considered at first this but it got confusing really fast. Take a look:
private final ActivationSystem systemStub;
SystemRegistryImpl(..., systemStub) {
...
this.systemStub = systemStub;
notifyAll();
...
}
private synchronized void awaitInitialized() {
...
while (systemStub == null) {
wait();
}
...
}
We rely on systemStub to be initialized at object creation time (before
construction!) to its default value of null. I think this is right. The
constructor then initializes the blank final to non-null and notifies.
Then, awaitInitialized seems straightforward until you see that the
condition is waiting for the value of a final variable to change! JLS
sec 17.5 [1] allows all sorts of optimizations for final fields, but
they all are qualified with what is essentially a safe publication
requirement on the reference:
An object is considered to be completely initialized when its
constructor
finishes. A thread that can only see a reference to an object after
that
object has been completely initialized is guaranteed to see the
correctly
initialized values for that object's final fields.
[1] http://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html#jls-17.5
Unfortunately this code *unsafely* publishes a reference to 'this' which
is the root of this whole problem. Under these circumstances I'd prefer
to be really conservative about the code here. I can't quite convince
myself that a condition loop waiting for a final field to change value
is safe. That's why I added a separate field.
I can see removing the boolean and using systemStub == null as the
condition making things simpler, since there are fewer variables to
reason about, but only if systemStub is made non-final. Making it
non-final prevents any unwanted optimizations resulting from it being
final. Having it be final doesn't add much anyway. I'll also move all
accesses to systemStub within synchronized blocks so there is no
question about visibility. (As a result, awaitInitialized now gets
turned into getSystemStub.)
(Peter) Should the interrupt break out of the loop even though the
condition isn't satisfied? This is a good point. Usually I think of
interrupt as wanting to avoid waiting indefinitely for the condition to
become true (which is the point of supporting interruption) but in this
case the condition will always occur in a timely fashion. So I'll accept
the suggestion to save the interrupt state and let the condition loop
terminate.
Updated webrev here:
http://cr.openjdk.java.net/~smarks/reviews/8023541/webrev.1/
Thanks,
s'marks
