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
