If no objections, I will push the changeset, and request to back the fix. Thanks, Xuelei
On 8/2/2013 9:12 AM, Xuelei Fan wrote: > On 8/2/2013 8:45 AM, Brad Wetmore wrote: >> >> >> On 7/25/2013 2:11 AM, Xuelei Fan wrote: >>> Hi Brad, >>> >>> Are you available to review this fix? >>> >>> Webrev: http://cr.openjdk.java.net/~xuelei/8013809/webrev.00/ >>> >>> No new regression test, hard to reproduce the issue. >> >> Your immediate fix looks good, however, IIRC, the reason for having >> getConnectionState() was to provide synchronized access (and syncing on >> the SSLSocketImpl object) to the connectionState variable before the >> Java "volatile" semantics were cleaned up in JDK 1.5. >> > I think there is a lot of different of a "volatile" object and a > synchronized block. A volatile object is to make sure the change and > access of the object is synchronized; but synchronized block also make > sure that the full process of the change is synchronized. For example: > > synchronized (aLock) { > // We want to change the object, but may need > // to prepare something before the change, please > // don't access this object while we make the preparation. > > ... // prepare something else. > > aObject = newvalue; > } > >> Now that you've made this change, does it make sense to get rid of >> getConnectionState(). What do you think? >> > I was hesitated to remove the getConnectionState(). When we change the > value of connectionState, "this" object is locked. If the value change > does not completed, other thread cannot access the state. If we remove > this synchronization, the behavior would be changed significantly. > > The reason that we can make the change in isClosed() implementation is > that "APP_CLOSED" is the final state of the life-cycle of connection, > therefore we don't care too much whether there is another thread is > doing the closing or not. It looks like a little tricky. > > Xuelei > >> I hate race conditions! ;) >> >> Brad >> >> >
