On Wed, 23 Apr 2025 11:36:37 GMT, Daniel Fuchs <dfu...@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). >> >> ### Improving HTTP/2 test server thread names >> >> The HTTP/2 server thread names are improved by modifying the server default >> executor to include the HTTP/2 server name in its thread names. We >> deliberately did nothing for the HTTP/1.1 server (provided by the >> `jdk.httpserver` module). `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_, >> introducing a name would require a public API change. >> Instead the calling code that creates the server can supply an Executor >> which creates threads with whatever name >> is appropriate for the application/test. > > 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()`. 1. What is wrong with `HttpTestServer::close` calling `HttpTestServer::stop`, which is already implemented as `impl.close()` by `Http2TestServerImpl`? 2. Shouldn't we better remove `implements AutoCloseable` and `close()` from `Http2TestServerImpl`? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/24822#discussion_r2056235392
