Setting ExecutorService causes connection leak

[olotenko]

3d04682#r45995045

@Path("/jax-rs-books")
@ApplicationScoped
public class JaxRsIndirectBookResource {
   @Inject
   protected Client client;

   @Inject
   @ConfigProperty(name = "book-service/mp-rest/url")
   protected String bookServiceUrl;

   @Inject
   @ConfigProperty(name = "sleep-service/mp-rest/url")
   protected String sleepServiceUrl;

   protected static final GenericType<Book[]> responseType = new GenericType(new Book[0].getClass());

   @GET
   @Path("/sync")
   @Produces(MediaType.APPLICATION_JSON)
   public Response getBooks() {
      Book[] books = client.target(bookServiceUrl + "/books")
                           .request(MediaType.APPLICATION_JSON)
                           .get(responseType);
      return Response.ok(books).build();
   }

   @GET
   @Path("/rx")
   @Produces(MediaType.APPLICATION_JSON)
   public void getBooksRx(@Suspended AsyncResponse response) {
      CompletionStage<Book[]> futureBooks =
         client.target(bookServiceUrl + "/books")
               .request(MediaType.APPLICATION_JSON)
               .rx()
               .get(responseType);
      futureBooks.thenApply(response::resume)
                 .exceptionally(response::resume);
   }
}

1. GET against /jax-rs-books/sync, one at a time, no concurrency needed - pooled connections get reused with all Connectors
2. GET against /jax-rs-books/rx, one at a time - pooled connections are not reused with Jetty and Netty connectors.

This can be seen to happen due to this (added an Exception that traces the creation point for NettyConnector, it is not really thrown, just logging):

java.lang.Exception: Eh?...New connector
	at org.glassfish.jersey.netty.connector.NettyConnector.<init>(NettyConnector.java:108)
	at org.glassfish.jersey.netty.connector.NettyConnectorProvider.getConnector(NettyConnectorProvider.java:56)
	at org.glassfish.jersey.client.ClientConfig$State.initRuntime(ClientConfig.java:459)
	at org.glassfish.jersey.internal.util.collection.Values$LazyValueImpl.get(Values.java:317)
	at org.glassfish.jersey.client.ClientConfig.getRuntime(ClientConfig.java:814)
	at org.glassfish.jersey.client.ClientRequest.getConfiguration(ClientRequest.java:220)
	at org.glassfish.jersey.client.JerseyInvocation.validateHttpMethodAndEntity(JerseyInvocation.java:132)
	at org.glassfish.jersey.client.JerseyInvocation.<init>(JerseyInvocation.java:92)
	at org.glassfish.jersey.client.JerseyInvocation.<init>(JerseyInvocation.java:88)
	at org.glassfish.jersey.client.JerseyInvocation.<init>(JerseyInvocation.java:77)
	at org.glassfish.jersey.client.JerseyInvocation$AsyncInvoker.method(JerseyInvocation.java:576)
	at org.glassfish.jersey.client.JerseyCompletionStageRxInvoker.method(JerseyCompletionStageRxInvoker.java:31)
	at org.glassfish.jersey.client.JerseyInvocation$AsyncInvoker.method(JerseyInvocation.java:546)
	at org.glassfish.jersey.client.AbstractNonSyncInvoker.get(AbstractNonSyncInvoker.java:33)
	at org.glassfish.jersey.client.JerseyCompletionStageRxInvoker.get(JerseyCompletionStageRxInvoker.java:31)
...

It seems that the culprit is line

jersey/core-client/src/main/java/org/glassfish/jersey/client/JerseyInvocation.java

Line 464 in 3d04682

request().getClientConfig().executorService(provided);

- commenting it out stops creation of new Connector for every request, and stops leaking the connections.

Perhaps, ClientConfig.State copying done as part of ClientConfig.executorService(provided) goes awry.

Reproducible on 2.33 and a few earlier builds. (Perhaps all the way to 2.31 where that line has been introduced, but I can't go below 2.31 because of the fixes that went into it)

[olotenko]

Well, at the time line 464 request().getClientConfig().executorService(provided); is executed, ClientConfig is in shared state, so it creates a copy on any modification. This copies everything, except LazyValue runtime, so it gets invoked next time runtime is accessed - which creates a new Client, and overwrites client.

It seems that if there should be some default, it may be better set very early in the ClientConfig lifetime. Then if no ExecutorService is set explicitly, it will use this default without having to update config at the time rx() is invoked.

[jansupol]

The origin comes from 2.29.1, where the fix provided the ExecutorService for a custom RxInvoker. While the code is not exactly correct, i.e. for Rx should be used ExecutorServiceProvider annotated with @ClientAsyncExecutor, as required by the runtime, we need to keep the priority of provided user ExecutorServiceProvider over an internal ClientAsyncExecutor annotated ExecutorServiceProvider for backward compatibility with the upper stack.

The behaviour you experience is as expected, for every connector Jersey supports. Whenever a property is changed or a provider is registered after the ClientRuntime has been instantiated, a new ClientRuntime with a new InjectionManager and a new Connector is created. Suppose a property that is used at the time of Connector creation. If the property is set after the Connector is created, a new Connector reflecting the property is created. Hence, connectors should be prepared for the situation that an instance is created multiple times and ought not to leak.

On the other hand, I agree that it is not necessary to recreate a new Connector for every rx() call. This is going to be fixed.