Commit 0214fe4b authored by Andy Wilkinson's avatar Andy Wilkinson

Remove inconsistent synchronization from EmbeddedWebApplicationContext

Previously, EmbeddedWebApplicationContext used synchronized, but did
not do so consistently. It also synchronized on this so its lock was
exposed outside of the class, creating a risk of deadlock if a caller
synchronized incorrectly. Furthermore, not all fields on the class
were sychronized so the class wasn't truly thread-safe.

This commit attempts to rectify some of the problems above. The use
of synchronized has been dropped in favour of using a volatile field
for the embedded servlet container. Whenever this field is accessed,
a local variable is used to "cache" the value thereby preventing a
change on another thread from causing unwanted behaviour such as an
NPE.

Closes gh-4593
parent 9bffdc80
...@@ -95,7 +95,7 @@ public class EmbeddedWebApplicationContext extends GenericWebApplicationContext ...@@ -95,7 +95,7 @@ public class EmbeddedWebApplicationContext extends GenericWebApplicationContext
*/ */
public static final String DISPATCHER_SERVLET_NAME = ServletContextInitializerBeans.DISPATCHER_SERVLET_NAME; public static final String DISPATCHER_SERVLET_NAME = ServletContextInitializerBeans.DISPATCHER_SERVLET_NAME;
private EmbeddedServletContainer embeddedServletContainer; private volatile EmbeddedServletContainer embeddedServletContainer;
private ServletConfig servletConfig; private ServletConfig servletConfig;
...@@ -138,10 +138,10 @@ public class EmbeddedWebApplicationContext extends GenericWebApplicationContext ...@@ -138,10 +138,10 @@ public class EmbeddedWebApplicationContext extends GenericWebApplicationContext
@Override @Override
protected void finishRefresh() { protected void finishRefresh() {
super.finishRefresh(); super.finishRefresh();
startEmbeddedServletContainer(); EmbeddedServletContainer localContainer = startEmbeddedServletContainer();
if (this.embeddedServletContainer != null) { if (localContainer != null) {
publishEvent(new EmbeddedServletContainerInitializedEvent(this, publishEvent(
this.embeddedServletContainer)); new EmbeddedServletContainerInitializedEvent(this, localContainer));
} }
} }
...@@ -151,15 +151,17 @@ public class EmbeddedWebApplicationContext extends GenericWebApplicationContext ...@@ -151,15 +151,17 @@ public class EmbeddedWebApplicationContext extends GenericWebApplicationContext
stopAndReleaseEmbeddedServletContainer(); stopAndReleaseEmbeddedServletContainer();
} }
private synchronized void createEmbeddedServletContainer() { private void createEmbeddedServletContainer() {
if (this.embeddedServletContainer == null && getServletContext() == null) { EmbeddedServletContainer localContainer = this.embeddedServletContainer;
ServletContext localServletContext = getServletContext();
if (localContainer == null && localServletContext == null) {
EmbeddedServletContainerFactory containerFactory = getEmbeddedServletContainerFactory(); EmbeddedServletContainerFactory containerFactory = getEmbeddedServletContainerFactory();
this.embeddedServletContainer = containerFactory this.embeddedServletContainer = containerFactory
.getEmbeddedServletContainer(getSelfInitializer()); .getEmbeddedServletContainer(getSelfInitializer());
} }
else if (getServletContext() != null) { else if (localServletContext != null) {
try { try {
getSelfInitializer().onStartup(getServletContext()); getSelfInitializer().onStartup(localServletContext);
} }
catch (ServletException ex) { catch (ServletException ex) {
throw new ApplicationContextException("Cannot initialize servlet context", throw new ApplicationContextException("Cannot initialize servlet context",
...@@ -284,16 +286,19 @@ public class EmbeddedWebApplicationContext extends GenericWebApplicationContext ...@@ -284,16 +286,19 @@ public class EmbeddedWebApplicationContext extends GenericWebApplicationContext
} }
} }
private void startEmbeddedServletContainer() { private EmbeddedServletContainer startEmbeddedServletContainer() {
if (this.embeddedServletContainer != null) { EmbeddedServletContainer localContainer = this.embeddedServletContainer;
this.embeddedServletContainer.start(); if (localContainer != null) {
localContainer.start();
} }
return localContainer;
} }
private synchronized void stopAndReleaseEmbeddedServletContainer() { private void stopAndReleaseEmbeddedServletContainer() {
if (this.embeddedServletContainer != null) { EmbeddedServletContainer localContainer = this.embeddedServletContainer;
if (localContainer != null) {
try { try {
this.embeddedServletContainer.stop(); localContainer.stop();
this.embeddedServletContainer = null; this.embeddedServletContainer = null;
} }
catch (Exception ex) { catch (Exception ex) {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment