On Thu, 1 Aug 2024 20:40:12 GMT, Anthony Scarpino <ascarp...@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
