On Wed, 23 Apr 2025 10:18:44 GMT, Volkan Yazici <vyaz...@openjdk.org> wrote:
> Incorporates the test server name while deriving the HTTP/2 test server > (i.e., `jdk.httpclient.test.lib.http2.Http2TestServer`) thread names to > improve diagnostics. > > ### Making `HttpTestServer` implement `AutoCloseable` > > I carried out this out-of-scope enhancement along with this PR, since this > one-liner gives nice try-with-resources convenience while writing tests using > HTTP test servers. Note that [this change is already implemented for the > in-progress HTTP/3 > work](/dfuch/jdk/blob/9c2da664d2875b7e7986831fd716d05b7a8306f4/test/jdk/java/net/httpclient/lib/jdk/httpclient/test/lib/common/HttpServerAdapters.java#L1119). > > ### Avoiding to improve HTTP/1.1 server thread names > > We deliberately kept improving HTTP/1.1 server (shipped by `jwebserver` and > delegated to for HTTP/1.1 server needs of `HttpClient` tests) thread names > out of the scope in this story. `ServerImpl`, the default HTTP/1.1 server > implementation, has only one thread by default: the dispatcher and it is > named `HTTP-Dispatcher`. Since factory methods in > `com.sun.net.httpserver.Http[s]Server` don't have the notion of a _name_, it > will be a rather big change to introduce a `name` there. We could have opted > for including host & port information in the `ServerImpl` dispatcher thread > name, but that might expose sensitive information. LGTM mostly. BTW - you might want to update the PR desctription: this PR is not avoiding to improve HTTP/1.1 server thread names, but rather improving HTTP/2 test server names ;-) test/jdk/java/net/httpclient/lib/jdk/httpclient/test/lib/common/HttpServerAdapters.java line 789: > 787: stop(); > 788: } > 789: The Http2TestServer is itself closeable, so you should override this method in Http2TestServerImpl below to call `impl.close()`. ------------- PR Review: https://git.openjdk.org/jdk/pull/24822#pullrequestreview-2786980597 PR Review Comment: https://git.openjdk.org/jdk/pull/24822#discussion_r2055857022
