From 8c896d93761999ad0de655e10ecdc5caf7fe0759 Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Thu, 4 Oct 2018 07:58:09 -0700 Subject: [PATCH] Improve Reactive...Context thread safety Refactor `ReactiveWebServerApplicationContext` to improve thread safety by using a single manager object rather than then trying to synchronize the `WebServer` and `HttpHandler`. Closes gh-14666 --- .../ReactiveWebServerApplicationContext.java | 138 +++++++++--------- ...ctiveWebServerApplicationContextTests.java | 6 +- 2 files changed, 76 insertions(+), 68 deletions(-) diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/reactive/context/ReactiveWebServerApplicationContext.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/reactive/context/ReactiveWebServerApplicationContext.java index a2e4227916b..75e5af057f6 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/reactive/context/ReactiveWebServerApplicationContext.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/reactive/context/ReactiveWebServerApplicationContext.java @@ -42,9 +42,7 @@ public class ReactiveWebServerApplicationContext extends GenericReactiveWebApplicationContext implements ConfigurableWebServerApplicationContext { - private volatile WebServer webServer; - - private volatile DeferredHttpHandler httpHandler; + private volatile ServerManager serverManager; private String serverNamespace; @@ -87,41 +85,13 @@ public class ReactiveWebServerApplicationContext } private void createWebServer() { - WebServer localServer = this.webServer; - if (localServer == null) { - DeferredHttpHandler localHandler = new DeferredHttpHandler( - this::getHttpHandler); - this.webServer = getWebServerFactory().getWebServer(localHandler); - this.httpHandler = localHandler; + ServerManager serverManager = this.serverManager; + if (serverManager == null) { + this.serverManager = ServerManager.get(getWebServerFactory()); } initPropertySources(); } - @Override - protected void finishRefresh() { - super.finishRefresh(); - WebServer localServer = startReactiveWebServer(); - if (localServer != null) { - publishEvent(new ReactiveWebServerInitializedEvent(localServer, this)); - } - } - - @Override - protected void onClose() { - super.onClose(); - stopAndReleaseReactiveWebServer(); - } - - /** - * Returns the {@link WebServer} that was created by the context or {@code null} if - * the server has not yet been created. - * @return the web server - */ - @Override - public WebServer getWebServer() { - return this.webServer; - } - /** * Return the {@link ReactiveWebServerFactory} that should be used to create the * reactive web server. By default this method searches for a suitable bean in the @@ -146,6 +116,21 @@ public class ReactiveWebServerApplicationContext return getBeanFactory().getBean(beanNames[0], ReactiveWebServerFactory.class); } + @Override + protected void finishRefresh() { + super.finishRefresh(); + WebServer webServer = startReactiveWebServer(); + if (webServer != null) { + publishEvent(new ReactiveWebServerInitializedEvent(webServer, this)); + } + } + + private WebServer startReactiveWebServer() { + ServerManager serverManager = this.serverManager; + ServerManager.start(serverManager, this::getHttpHandler); + return ServerManager.getWebServer(serverManager); + } + /** * Return the {@link HttpHandler} that should be used to process the reactive web * server. By default this method searches for a suitable bean in the context itself. @@ -166,30 +151,30 @@ public class ReactiveWebServerApplicationContext return getBeanFactory().getBean(beanNames[0], HttpHandler.class); } - private WebServer startReactiveWebServer() { - WebServer localServer = this.webServer; - DeferredHttpHandler localHandler = this.httpHandler; - if (localServer != null) { - if (localHandler != null) { - localHandler.initialize(); - this.httpHandler = null; - } - localServer.start(); - } - return localServer; + @Override + protected void onClose() { + super.onClose(); + stopAndReleaseReactiveWebServer(); } private void stopAndReleaseReactiveWebServer() { - WebServer localServer = this.webServer; - if (localServer != null) { - try { - localServer.stop(); - this.webServer = null; - } - catch (Exception ex) { - throw new IllegalStateException(ex); - } + ServerManager serverManager = this.serverManager; + try { + ServerManager.stop(serverManager); } + finally { + this.serverManager = null; + } + } + + /** + * Returns the {@link WebServer} that was created by the context or {@code null} if + * the server has not yet been created. + * @return the web server + */ + @Override + public WebServer getWebServer() { + return ServerManager.getWebServer(this.serverManager); } @Override @@ -203,22 +188,18 @@ public class ReactiveWebServerApplicationContext } /** - * {@link HttpHandler} that defers to a supplied handler which is initialized only - * when the server starts. + * Internal class used to manage the server and the {@link HttpHandler}, taking care + * not to initialize the hander too early. */ - static class DeferredHttpHandler implements HttpHandler { + static final class ServerManager implements HttpHandler { - private Supplier factory; + private final WebServer server; - private HttpHandler handler; + private volatile HttpHandler handler; - DeferredHttpHandler(Supplier factory) { - this.factory = factory; + private ServerManager(ReactiveWebServerFactory factory) { this.handler = this::handleUninitialized; - } - - public void initialize() { - this.handler = this.factory.get(); + this.server = factory.getWebServer(this); } private Mono handleUninitialized(ServerHttpRequest request, @@ -236,6 +217,33 @@ public class ReactiveWebServerApplicationContext return this.handler; } + public static ServerManager get(ReactiveWebServerFactory factory) { + return new ServerManager(factory); + } + + public static WebServer getWebServer(ServerManager manager) { + return (manager != null) ? manager.server : null; + } + + public static void start(ServerManager manager, + Supplier handlerSupplier) { + if (manager != null) { + manager.handler = handlerSupplier.get(); + manager.server.start(); + } + } + + public static void stop(ServerManager manager) { + if (manager != null) { + try { + manager.server.stop(); + } + catch (Exception ex) { + throw new IllegalStateException(ex); + } + } + } + } } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/reactive/context/AnnotationConfigReactiveWebServerApplicationContextTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/reactive/context/AnnotationConfigReactiveWebServerApplicationContextTests.java index 6556fe697ca..134d63ab747 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/reactive/context/AnnotationConfigReactiveWebServerApplicationContextTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/reactive/context/AnnotationConfigReactiveWebServerApplicationContextTests.java @@ -18,7 +18,7 @@ package org.springframework.boot.web.reactive.context; import org.junit.Test; -import org.springframework.boot.web.reactive.context.ReactiveWebServerApplicationContext.DeferredHttpHandler; +import org.springframework.boot.web.reactive.context.ReactiveWebServerApplicationContext.ServerManager; import org.springframework.boot.web.reactive.context.config.ExampleReactiveWebServerApplicationConfiguration; import org.springframework.boot.web.reactive.server.MockReactiveWebServerFactory; import org.springframework.boot.web.reactive.server.ReactiveWebServerFactory; @@ -98,8 +98,8 @@ public class AnnotationConfigReactiveWebServerApplicationContextTests { .getBean(MockReactiveWebServerFactory.class); HttpHandler expectedHandler = this.context.getBean(HttpHandler.class); HttpHandler actualHandler = factory.getWebServer().getHttpHandler(); - if (actualHandler instanceof DeferredHttpHandler) { - actualHandler = ((DeferredHttpHandler) actualHandler).getHandler(); + if (actualHandler instanceof ServerManager) { + actualHandler = ((ServerManager) actualHandler).getHandler(); } assertThat(actualHandler).isEqualTo(expectedHandler); }