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
This commit is contained in:
Phillip Webb 2018-10-04 07:58:09 -07:00
parent cf24d18139
commit 8c896d9376
2 changed files with 76 additions and 68 deletions

View File

@ -42,9 +42,7 @@ public class ReactiveWebServerApplicationContext
extends GenericReactiveWebApplicationContext extends GenericReactiveWebApplicationContext
implements ConfigurableWebServerApplicationContext { implements ConfigurableWebServerApplicationContext {
private volatile WebServer webServer; private volatile ServerManager serverManager;
private volatile DeferredHttpHandler httpHandler;
private String serverNamespace; private String serverNamespace;
@ -87,41 +85,13 @@ public class ReactiveWebServerApplicationContext
} }
private void createWebServer() { private void createWebServer() {
WebServer localServer = this.webServer; ServerManager serverManager = this.serverManager;
if (localServer == null) { if (serverManager == null) {
DeferredHttpHandler localHandler = new DeferredHttpHandler( this.serverManager = ServerManager.get(getWebServerFactory());
this::getHttpHandler);
this.webServer = getWebServerFactory().getWebServer(localHandler);
this.httpHandler = localHandler;
} }
initPropertySources(); 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 * 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 * 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); 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 * 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. * 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); return getBeanFactory().getBean(beanNames[0], HttpHandler.class);
} }
private WebServer startReactiveWebServer() { @Override
WebServer localServer = this.webServer; protected void onClose() {
DeferredHttpHandler localHandler = this.httpHandler; super.onClose();
if (localServer != null) { stopAndReleaseReactiveWebServer();
if (localHandler != null) {
localHandler.initialize();
this.httpHandler = null;
}
localServer.start();
}
return localServer;
} }
private void stopAndReleaseReactiveWebServer() { private void stopAndReleaseReactiveWebServer() {
WebServer localServer = this.webServer; ServerManager serverManager = this.serverManager;
if (localServer != null) { try {
try { ServerManager.stop(serverManager);
localServer.stop();
this.webServer = null;
}
catch (Exception ex) {
throw new IllegalStateException(ex);
}
} }
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 @Override
@ -203,22 +188,18 @@ public class ReactiveWebServerApplicationContext
} }
/** /**
* {@link HttpHandler} that defers to a supplied handler which is initialized only * Internal class used to manage the server and the {@link HttpHandler}, taking care
* when the server starts. * not to initialize the hander too early.
*/ */
static class DeferredHttpHandler implements HttpHandler { static final class ServerManager implements HttpHandler {
private Supplier<HttpHandler> factory; private final WebServer server;
private HttpHandler handler; private volatile HttpHandler handler;
DeferredHttpHandler(Supplier<HttpHandler> factory) { private ServerManager(ReactiveWebServerFactory factory) {
this.factory = factory;
this.handler = this::handleUninitialized; this.handler = this::handleUninitialized;
} this.server = factory.getWebServer(this);
public void initialize() {
this.handler = this.factory.get();
} }
private Mono<Void> handleUninitialized(ServerHttpRequest request, private Mono<Void> handleUninitialized(ServerHttpRequest request,
@ -236,6 +217,33 @@ public class ReactiveWebServerApplicationContext
return this.handler; 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<HttpHandler> 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);
}
}
}
} }
} }

View File

@ -18,7 +18,7 @@ package org.springframework.boot.web.reactive.context;
import org.junit.Test; 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.context.config.ExampleReactiveWebServerApplicationConfiguration;
import org.springframework.boot.web.reactive.server.MockReactiveWebServerFactory; import org.springframework.boot.web.reactive.server.MockReactiveWebServerFactory;
import org.springframework.boot.web.reactive.server.ReactiveWebServerFactory; import org.springframework.boot.web.reactive.server.ReactiveWebServerFactory;
@ -98,8 +98,8 @@ public class AnnotationConfigReactiveWebServerApplicationContextTests {
.getBean(MockReactiveWebServerFactory.class); .getBean(MockReactiveWebServerFactory.class);
HttpHandler expectedHandler = this.context.getBean(HttpHandler.class); HttpHandler expectedHandler = this.context.getBean(HttpHandler.class);
HttpHandler actualHandler = factory.getWebServer().getHttpHandler(); HttpHandler actualHandler = factory.getWebServer().getHttpHandler();
if (actualHandler instanceof DeferredHttpHandler) { if (actualHandler instanceof ServerManager) {
actualHandler = ((DeferredHttpHandler) actualHandler).getHandler(); actualHandler = ((ServerManager) actualHandler).getHandler();
} }
assertThat(actualHandler).isEqualTo(expectedHandler); assertThat(actualHandler).isEqualTo(expectedHandler);
} }