From 0568d3259aefa0963edf95894816d31e095929af Mon Sep 17 00:00:00 2001 From: Graeme Rocher Date: Thu, 6 Jun 2024 18:07:11 +0200 Subject: [PATCH 1/2] Correct clients getting shutdown by request scope --- .../http/client/netty/ConnectionManager.java | 65 +++++++++++-------- .../http/scope/RequestScopeSpec.groovy | 26 +++++++- .../micronaut/context/DefaultBeanContext.java | 2 +- 3 files changed, 61 insertions(+), 32 deletions(-) diff --git a/http-client/src/main/java/io/micronaut/http/client/netty/ConnectionManager.java b/http-client/src/main/java/io/micronaut/http/client/netty/ConnectionManager.java index bdeaacd690f..f110763af39 100644 --- a/http-client/src/main/java/io/micronaut/http/client/netty/ConnectionManager.java +++ b/http-client/src/main/java/io/micronaut/http/client/netty/ConnectionManager.java @@ -136,6 +136,8 @@ public class ConnectionManager { private final ClientSslBuilder nettyClientSslBuilder; private EventLoopGroup group; private final boolean shutdownGroup; + + private final AtomicBoolean running = new AtomicBoolean(false); private final ThreadFactory threadFactory; private final ChannelFactory socketChannelFactory; private final ChannelFactory udpChannelFactory; @@ -206,7 +208,9 @@ public class ConnectionManager { shutdownGroup = true; } + refresh(); + running.set(true); } final void refresh() { @@ -315,10 +319,12 @@ final int liveRequestCount() { * @see DefaultHttpClient#start() */ public final void start() { - // only need to start new group if it's managed by us - if (shutdownGroup) { - group = createEventLoopGroup(configuration, threadFactory); - initBootstrap(); // rebuild bootstrap with new group + if (running.compareAndSet(false, true)) { + // only need to start new group if it's managed by us + if (shutdownGroup) { + group = createEventLoopGroup(configuration, threadFactory); + initBootstrap(); // rebuild bootstrap with new group + } } } @@ -352,31 +358,34 @@ private void initBootstrap() { * @see DefaultHttpClient#stop() */ public final void shutdown() { - for (Pool pool : pools.values()) { - pool.shutdown(); - } - if (shutdownGroup) { - Duration shutdownTimeout = configuration.getShutdownTimeout() - .orElse(Duration.ofMillis(HttpClientConfiguration.DEFAULT_SHUTDOWN_TIMEOUT_MILLISECONDS)); - Duration shutdownQuietPeriod = configuration.getShutdownQuietPeriod() - .orElse(Duration.ofMillis(HttpClientConfiguration.DEFAULT_SHUTDOWN_QUIET_PERIOD_MILLISECONDS)); - - Future future = group.shutdownGracefully( - shutdownQuietPeriod.toMillis(), - shutdownTimeout.toMillis(), - TimeUnit.MILLISECONDS - ); - try { - future.await(shutdownTimeout.toMillis()); - } catch (InterruptedException e) { - // ignore - Thread.currentThread().interrupt(); + if (running.compareAndSet(true, false)) { + + for (Pool pool : pools.values()) { + pool.shutdown(); } + if (shutdownGroup) { + Duration shutdownTimeout = configuration.getShutdownTimeout() + .orElse(Duration.ofMillis(HttpClientConfiguration.DEFAULT_SHUTDOWN_TIMEOUT_MILLISECONDS)); + Duration shutdownQuietPeriod = configuration.getShutdownQuietPeriod() + .orElse(Duration.ofMillis(HttpClientConfiguration.DEFAULT_SHUTDOWN_QUIET_PERIOD_MILLISECONDS)); + + Future future = group.shutdownGracefully( + shutdownQuietPeriod.toMillis(), + shutdownTimeout.toMillis(), + TimeUnit.MILLISECONDS + ); + try { + future.await(shutdownTimeout.toMillis()); + } catch (InterruptedException e) { + // ignore + Thread.currentThread().interrupt(); + } + } + ReferenceCountUtil.release(sslContext); + ReferenceCountUtil.release(websocketSslContext); + sslContext = null; + websocketSslContext = null; } - ReferenceCountUtil.release(sslContext); - ReferenceCountUtil.release(websocketSslContext); - sslContext = null; - websocketSslContext = null; } /** @@ -385,7 +394,7 @@ public final void shutdown() { * @return Whether this connection manager is still running and can serve requests */ public final boolean isRunning() { - return !group.isShutdown(); + return running.get() && !group.isShutdown(); } /** diff --git a/http-server-netty/src/test/groovy/io/micronaut/runtime/http/scope/RequestScopeSpec.groovy b/http-server-netty/src/test/groovy/io/micronaut/runtime/http/scope/RequestScopeSpec.groovy index ae29c952ce2..3ff9c0f5917 100644 --- a/http-server-netty/src/test/groovy/io/micronaut/runtime/http/scope/RequestScopeSpec.groovy +++ b/http-server-netty/src/test/groovy/io/micronaut/runtime/http/scope/RequestScopeSpec.groovy @@ -20,8 +20,12 @@ import io.micronaut.context.event.ApplicationEventListener import io.micronaut.http.HttpRequest import io.micronaut.http.annotation.Controller import io.micronaut.http.annotation.Get +import io.micronaut.http.client.HttpClient +import io.micronaut.http.client.annotation.Client import io.micronaut.http.context.event.HttpRequestTerminatedEvent import io.micronaut.http.server.netty.AbstractMicronautSpec +import io.micronaut.scheduling.TaskExecutors +import io.micronaut.scheduling.annotation.ExecuteOn import jakarta.annotation.PreDestroy import jakarta.inject.Inject import jakarta.inject.Singleton @@ -174,13 +178,17 @@ class RequestScopeSpec extends AbstractMicronautSpec { static class SimpleRequestBean { private final SimpleBean simpleBean + final HttpClient client - SimpleRequestBean(SimpleBean simpleBean) { + SimpleRequestBean( + SimpleBean simpleBean, + @Client("/") HttpClient client) { this.simpleBean = simpleBean + this.client = client } String sayHello() { - return "HELLO" + return client.toBlocking().retrieve("/test-simple-request-scope-other") } } @@ -200,15 +208,27 @@ class RequestScopeSpec extends AbstractMicronautSpec { @Controller static class SimpleTestController { final SimpleRequestBean simpleRequestBean + final HttpClient client - SimpleTestController(SimpleRequestBean simpleRequestBean) { + SimpleTestController( + SimpleRequestBean simpleRequestBean, + @Client("/") HttpClient client) { this.simpleRequestBean = simpleRequestBean + this.client = client } @Get("/test-simple-request-scope") + @ExecuteOn(TaskExecutors.BLOCKING) String test() { return simpleRequestBean.sayHello() } + + @Get("/test-simple-request-scope-other") + String test2() { + simpleRequestBean.client.is(client) + assert simpleRequestBean.client.isRunning() + return "HELLO" + } } @RequestScope diff --git a/inject/src/main/java/io/micronaut/context/DefaultBeanContext.java b/inject/src/main/java/io/micronaut/context/DefaultBeanContext.java index fe2e27f534c..b8a83dfc41d 100644 --- a/inject/src/main/java/io/micronaut/context/DefaultBeanContext.java +++ b/inject/src/main/java/io/micronaut/context/DefaultBeanContext.java @@ -1204,7 +1204,7 @@ private void destroyBean(@NonNull BeanRegistration registration, boolean } } } - if (beanToDestroy instanceof LifeCycle cycle) { + if (beanToDestroy instanceof LifeCycle cycle && !dependent) { destroyLifeCycleBean(cycle, definition); } if (registration instanceof BeanDisposingRegistration) { From c8894b554b6d1db0512e8541380124bc789fb34e Mon Sep 17 00:00:00 2001 From: Graeme Rocher Date: Thu, 6 Jun 2024 18:12:51 +0200 Subject: [PATCH 2/2] move running set call --- .../java/io/micronaut/http/client/netty/ConnectionManager.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/http-client/src/main/java/io/micronaut/http/client/netty/ConnectionManager.java b/http-client/src/main/java/io/micronaut/http/client/netty/ConnectionManager.java index f110763af39..75090391cec 100644 --- a/http-client/src/main/java/io/micronaut/http/client/netty/ConnectionManager.java +++ b/http-client/src/main/java/io/micronaut/http/client/netty/ConnectionManager.java @@ -210,7 +210,6 @@ public class ConnectionManager { refresh(); - running.set(true); } final void refresh() { @@ -228,6 +227,7 @@ final void refresh() { http3SslContext = null; } initBootstrap(); + running.set(true); for (Pool pool : pools.values()) { pool.forEachConnection(c -> ((Pool.ConnectionHolder) c).windDownConnection()); }