On Mon, 29 Jul 2024 17:52:52 GMT, Daniel Jeliński <djelin...@openjdk.org> wrote:
>> Anthony Scarpino has updated the pull request with a new target base due to >> a merge or a rebase. The pull request now contains 21 commits: >> >> - Rework TLSBase for better testing >> - Tests working >> - Merge branch 'master' into nst-multi >> - new changes >> - remove frag issue >> - Comments, remove thread, set NST default to 1, allow 0 >> - comment cleanup >> - Merge branch 'master' into nst-multi >> - copyright & cleanup >> - oops BAOS >> - ... and 11 more: https://git.openjdk.org/jdk/compare/3796fdfc...35bfe799 > > 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. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1700815795
