RFR(s): 8023541 Race condition in rmid initialization

Daniel Fuchs daniel.fuchs at oracle.com
Thu Jan 30 09:09:33 UTC 2014

Hi Stuart,

I wonder whether you should replace the assert in the
constructor by an explicit null check:

-  assert systemStub != null
+ if (systemStub == null) throw new NullPointerException();

The reason I see is that before your change, an object constructed
with a null systemStub would have sooner or later failed in NPE.
Now with your change, an object constructed with a null system
stub will block - waiting forever for system stub to become not

The question of course is whether throwing NPE in  the constructor
would cause any compatibility issues. Passing the JCK might help
to figure it out.

Best regards,

-- daniel

On 1/30/14 3:57 AM, 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

More information about the core-libs-dev mailing list