On Sat, 3 Aug 2024 00:46:05 GMT, Anthony Scarpino <ascarp...@openjdk.org> wrote:
>> Hi >> >> This change is to improve TLS 1.3 session resumption by allowing a TLS >> server to send more than one resumption ticket per connection and clients to >> store more. Resumption is a quick way to use an existing TLS session to >> establish another session by avoiding the long TLS full handshake process. >> In TLS 1.2 and below, clients can repeatedly resume a session by using the >> session ID from an established connection. In TLS 1.3, a one-time >> "resumption ticket" is sent by the server after the TLS connection has been >> established. The server may send multiple resumption tickets to help >> clients that rapidly resume connections. If the client does not have >> another resumption ticket, it must go through the full TLS handshake again. >> The current implementation in JDK 23 and below, only sends and store one >> resumption ticket. >> >> The number of resumption tickets a server can send should be configurable by >> the application developer or administrator. [RFC >> 8446](https://www.rfc-editor.org/rfc/rfc8446) does not specify a default >> value. A system property called `jdk.tls.server.newSessionTicketCount` >> allows the user to change the number of resumption tickets sent by the >> server. If this property is not set or given an invalid value, the default >> value of 3 is used. Further details are in the CSR. >> >> A large portion of the changeset is on the client side by changing the >> caching system used by TLS. It creates a new `CacheEntry<>` type called >> `QueueCacheEntry<>` that will store multiple values for a Map entry. > > Anthony Scarpino has updated the pull request incrementally with two > additional commits since the last revision: > > - revert to synchronized > - code review changes src/java.base/share/classes/sun/security/util/Cache.java line 367: > 365: // If this is a queue, check for some expired entries > 366: if (entry instanceof QueueCacheEntry<K,V> qe) { > 367: qe.getQueue().removeIf(e -> e.isValid(time)); Shouldn't this be `e -> !e.isValid(time)`? src/java.base/share/classes/sun/security/util/Cache.java line 468: > 466: } > 467: > 468: synchronized public V get(Object key) { please revert the modifier order change src/java.base/share/classes/sun/security/util/Cache.java line 476: > 474: > 475: if (lifetime > 0 && !entry.isValid(System.currentTimeMillis())) { > 476: removeImpl(key); you can revert to `cacheMap.remove(key);`; `invalidate` is already done by `isValid` src/java.base/share/classes/sun/security/util/Cache.java line 478: > 476: removeImpl(key); > 477: if (DEBUG) { > 478: System.out.println("Ignoring expired entry: "); did you intend to print the entry contents here? src/java.base/share/classes/sun/security/util/Cache.java line 494: > 492: } > 493: > 494: public void remove(Object key) { this still needs to be synchronized because of `emptyQueue` src/java.base/share/classes/sun/security/util/Cache.java line 499: > 497: } > 498: > 499: private synchronized void removeImpl(Object key) { once you change `remove`, this doesn't need to be synchronized, because all callers are already synchronized. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1722244026 PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1722247219 PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1722252662 PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1722251527 PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1722253359 PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1722254245
