On Wed, 22 Jan 2025 12:05:40 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
> Can I please get a review of this test-only change which proposes to address > an intermittent failure in the > `test/jdk/java/net/httpclient/HttpClientSNITest.java` test? > > This is a recently introduced test and has been implemented to verify that > the right SNI server_name(s) get passed from the `HttpClient` to the server > during a TLS handshake. The test methods have been implemented in a way that > both the client and the servers are started afresh so that the TLS handshake > is guaranteed to happen for the connection that is established for the HTTP > request issued by the test methods. However, there was an oversight in the > test methods - although they create the client and the server afresh, the > `SSLContext` that was used by the client (and the server) is created only > once for the lifetime of the test class and then shared for each of these > test method runs. > > The internal implementation of `SSLContext` has the ability to resume (i.e. > reuse) TLS sessions against the same remote peer host:port combination. If > the same `SSLContext` is used across multiple test methods, then it can allow > for the case where an already established TLS session is reused for a HTTP > connection. This goes against the expectation of the test methods in this > test and can cause failures. > > This test failed (only) once in our CI and when that happened, it was noticed > that the second server which was created afresh in one of the test methods > and bound through an ephemeral port, ended up being allocated the same port > that was allocated to the first server instance (the one which was already > shutdown). Such allocation is allowed and there's nothing wrong with it. When > that happened, the shared `SSLContext` instance, I believe, ended up reusing > a previously established TLS session and thus the test method failed. > > The commit in this PR creates and uses a new instance of `SSLContext` for > every run of the test method, so that TLS sessions aren't resumed. > > The test continues to pass with this change. LGTM. ------------- Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/23233#pullrequestreview-2567162646
