RFR: 8328608: Multiple NewSessionTicket support for TLS [v3]

[Daniel Jeliński]

On Thu, 1 Aug 2024 20:40:12 GMT, Anthony Scarpino <ascarpino at openjdk.org> wrote:

>> src/java.base/share/classes/sun/security/util/Cache.java line 280:
>> 
>>> 278:     // Locking is to protect QueueCacheEntry's from being removed from the
>>> 279:     // cacheMap while another thread is adding new queue entries.
>>> 280:     private ReentrantLock lock;
>> 
>> The cache currently uses `synchronized` for all data accesses (put, get, remove); please use synchronized instead of this lock.
>
> Maybe I'm missing something you're saying here but I don't think the lock is wrong.  get() is no longer `synchronized` and some of the methods were changed.  Additionally, with `QueueCacheEntry` in a Map, multiple threads could be accessing the entry at the same time.
> The scenario I thought of was two threads use the same queue entry, which is expired or empty.  One thread uses `get()`, the other `put()`.  There is a chance where the get thread is in the process of removing the entry from the Map as the put thread is updating the entry.  This could result in the new PSKs being lost.
> If there was no locking, I believe the worse would be a full handshake on resumption, which maybe an acceptable loss.  But Cache is used elsewhere, so I felt this should prevent losing data in other situations where all data is important.

Every method that accesses the cacheMap needs to be synchronized using the same synchronization method. As it stands now, `clear` and `remove` are synchronized, `get` uses the lock, `put` uses both, and `expungeExpiredEntries` is sometimes called without any synchronization.

Note that reads (`get`, `size`) must be synchronized with writes. There are reports or `get` ending up in an infinite loop when called without synchronization.

I recommended reverting everything to `synchronized` because that makes the PR smaller. I guess you could replace all uses of `synchronized` with a ReentrantLock, but I don't see any benefit in doing that.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1701397145