RFR(s): 8023541 Race condition in rmid initialization

[David Holmes]

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