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

[Daniel Fuchs]

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).

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'?

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

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?

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