RFR [13] JDK-8221882: Use fiber-friendly java.util.concurrent.locks in JSSE

[Xuelei Fan]

Hi Daniel & Alan,

All good catches!  I updated accordingly:
   http://cr.openjdk.java.net/~xuelei/8221882/webrev.03/


On 4/4/2019 3:01 AM, Daniel Fuchs wrote:
> Hi Xuelei,
> 
> I second Alan's suggestion to use VarHandle CAS for getDefault().
> Otherwise you will have a race between getDefault() and setDefault()
> which will be problematic (and could make tests/app fail randomly).
> 
Updated to use var handler.

> WRT default factories I like the new holder class.
> I wonder if you should bite the bullet and take this opportunity
> to get rid of the @SuppressWarnings("deprecation") by replacing
> calls to Class::newInstance by calls to Class::getConstructor,
> as was done elsewhere in the JDK?
> 
> HttpsURLConnectionImpl.java:
> 
> If delegate is not final it should now be volatile.
> I am not seeing any subclasses of HttpsURLConnectionImpl in JDK 13.
> I guess 'plugin' meant 'plugin for applets'?
> 
Yes.  I removed 'plugin' stuffs in the update.  BTW, it is not necessary 
to use delegate mode of the HttpsURLConnection impl since the code 
clean-up in JDK-8215430.  We may want an additional 
HttpsURLConnectionImpl clean-up later in a new bug.

> InputRecord.java
> 
> Given that isClosed can only transition from `false` to `true`
> you could add a fastpath to isClosed():
> 
>     if (isClosed) return true;
>     // otherwise lock and look at the value again
> 
Good point.

> OutputRecord.java and SSLEngineOutputRecord.java:
> 
> I was wondering why you didn't use the lock in isClosed(), and
> noticed that OutputRecord declares isClosed as volatile.
> Which makes me wonder if InputRecord should do the same, and
> just forget the lock in InputRecord::isClosed?
> 
Good catch.  I updated to use violate isClosed in InputRecord as well.

Thanks,
Xuelei

> Otherwise it looks all reasonable to me.
> 
> best regards,
> 
> -- daniel
> 
> 
> 
> On 03/04/2019 20:33, Xuelei Fan wrote:
>> On 4/3/2019 12:06 PM, Alan Bateman wrote:
>>> On 03/04/2019 19:40, Xuelei Fan wrote:
>>>> Updated webrev (updated SSLContext.java, HttpsClient, 
>>>> HttpsURLConnectionImpl):
>>>>    http://cr.openjdk.java.net/~xuelei/8221882/webrev.01/
>>>>
>>> This looks good to me, the only change that I'm not 100% sure on is 
>>> in HttpsClient where the locking will not be consistent with the 
>>> locking in the HttpClient super class. Would you mind doubling 
>>> checking that one? We do need to change the HTTP protocol handler so 
>>> I expect HttpClient will change but it may have to add a protected 
>>> lock that HttpsClient uses to ensure that they are using the same lock.
>>>
>> Hm, the update is problematic.  Thanks for the catching!
>>
>> It might be better to make the update together with other http code in 
>> another enhancement.  I'd like not update the HttpsClient update this 
>> time.
>>
>> new webrev (remove update for HttpsClient.java):
>>     http://cr.openjdk.java.net/~xuelei/8221882/webrev.02/
>>
>> Thanks,
>> Xuelei
>