RFR(s): 8023541 Race condition in rmid initialization

[Stuart Marks]

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