RFR: 8241248: NullPointerException in sun.security.ssl.HKDF.extract(HKDF.java:93)

[Xue-Lei Andrew Fan]

On Tue, 27 Apr 2021 23:45:30 GMT, Alexey Bakhtin <abakhtin at openjdk.org> wrote:

>> src/java.base/share/classes/sun/security/ssl/PreSharedKeyExtension.java line 377:
>> 
>>> 375:                     // If we are keeping state, see if the identity is in the cache
>>> 376:                     if (requestedId.identity.length == SessionId.MAX_LENGTH) {
>>> 377:                         synchronized (sessionCache) {
>> 
>> Did you have a test if there is a performance regression by introducing the synchronization here?
>> 
>> I have to check the engineGetServerSessionContext() specification and implementation (if the sessionCache is a reference?), and the session cache implementation to make sure the synchronization works (if the synchronization on sessionCache is the same as the synchronization on the cache internal implementation) .  Maybe, the improvement could in the cache implementation for readability?
>
> Yes, I’ve made a test that calculates total time spent by server to receive "N" connections. Every server handshake is performed in a separate thread
> The client starts "T" threads. Every thread sends one initial connection and "R-1" renegotiated connections. So, the total number of connections is  "N"="T"*"R"
> 
> Results for the original and JDK-8241248 code are almost identical:
> "T"=10 "R"=100
> Original OpenJDK: 1140 ms
> JDK-8241248 code: 1090 ms
> 
> "T"=50 "R"=100
> Original OpenJDK: 1164 ms
> JDK-8241248 code: 1108 ms
> 
> "T"=60 "R"=100
> Original OpenJDK: 1461 ms
> JDK-8241248 code: 1579 ms
> 
> "T"=70 "R"=100
> Original OpenJDK: 2058 ms
> JDK-8241248 code: 1999 ms
> 
> "T"=80 "R"=100
> Original OpenJDK: 2148 ms
> JDK-8241248 code: 2125 ms
> 
> "T"=90 "R"=100
> Original OpenJDK: 2540 ms
> JDK-8241248 code: 2514 ms
> 
> "T"=90 "R"=100
> Original OpenJDK: 3011 ms
> JDK-8241248 code: 3010 ms
> 
> I can confirm that the synchronization code works. Without it, I still catch exception from different threads trying to resume the same session from the cache

Thank you for the update.

I would like to have the synchronization in the cache class for easier maintainence.  But please go ahead for the integration if you don't want to make the update now. We could file an enhancement later on.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3664