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

[Xuelei Fan]

On 4/3/2019 2:32 AM, Bernd Eckenfels wrote:
> Hello,
> 
> There are a few places where a synchronized method is freed up w/o new 
> lock, which is generally a good thing but I wonder if there is a 
> justification available why it is no problem (DTLSInputRecord vs. 
> DTLSOutputRecord).
> 
For DTLSInputRecord.close(), the isClosed variable cannot be reset from 
true to false, so it is safe to use the lock in super.close();

For DTLSOutputRecord, the close() method may set the "isCloseWaiting" 
filed, so additional lock is required here.

> Is the DCL In EphemeralKepair Safe, I am not sure how arrays and not 
> beeing volatile mixes?
> 
I think it is safe as once the array items get set, it won't change.  As 
the set process is locked and double checked, it is not necessary to use 
volatile modifier for the array items.

> Is there an undesired spacing change in a  SSlEngineImpl.java and 
> SSLSocketOutputRecord.java (or is this a webrev artifact)?
> 
Would you mind tell me the line numbers?  I did not catch them.  Might 
be a webrev artifact.

> The additional use of the SSLEngineOutputRecord lock in close, is this 
> fine? (It could block the close() if the lock is held)
> 
Good catch.  It could be a potential issue. I'm working on it in anther 
bug.  I will not touch the issue as the update is not as straightforward 
as I can see now.

> From a commit description point of few removing of unneeded 
> Synchronisation or replacing them with a holder pattern and introducing 
> DCL is a very positive thing which should be mentioned in the 
> description I think?
> 
Good point!  I added more description in another reply in the thread.

> And an aside, is this a general direction the JCL has to go for loom to 
> remove synchronized methods and blocks? Those ex synchronized methods 
> look a lot more ugly with the try/final (especially since there is no 
> TWR autoclose) and the additional locks also increase the footprint.
> 
Loom is still in progress. I'm not sure of the final spec yet.

Thanks for review!

Xuelei

> Gruss
> Bernd
> -- 
> http://bernd.eckenfels.net
> ------------------------------------------------------------------------
> *Von:* security-dev <security-dev-bounces at openjdk.java.net> im Auftrag 
> von Xuelei Fan <xuelei.fan at oracle.com>
> *Gesendet:* Mittwoch, April 3, 2019 6:48 AM
> *An:* security-dev at openjdk.java.net; Alan Bateman
> *Betreff:* RFR [13] JDK-8221882: Use fiber-friendly 
> java.util.concurrent.locks in JSSE
> Hi,
> 
> Could I get the following update reviewed?
> http://cr.openjdk.java.net/~xuelei/8221882/webrev.00/
> 
> To benefits from with Fibers [1], there is a need to use explicit locks,
> java.util.concurrent.locks, for synchronization in JSSE and the SunJSSE
> provider.
> 
> Most of the update is replacing synchronized blocks with
> java.util.concurrent.locks.ReentrantLock locking/unlocking.
> 
> Thanks,
> Xuelei
> [1]: http://cr.openjdk.java.net/~rpressler/loom/Loom-Proposal.html