Eclipse
/
jetty.project
mirror of https://github.com/jetty/jetty.project.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Issue #5088 Review ContextHandler locking (#5094) 

* Issue #5088 Review ContextHandler locking

The locking was primarily as a memory guard for the availability status, which was already volatile.
Have instead using an AtomicReference with a simple state machine layered on top of start/stop lifecycle.
There was also protection for AttributesMap, which is no longer needed as AttributesMap is now concurrent.

* Issue #5088

updates from review

* Issue #5088

updates from review (better this time)
This commit is contained in:
Greg Wilkins 2020-07-30 17:58:34 +02:00 committed by GitHub
parent 8b52d7dfcb
commit 66ec16006e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 101 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
jetty-server/src/main/java/org/eclipse/jetty/server/Request.java
View File

@ -1805,7 +1805,7 @@ public class Request implements HttpServletRequest
}
else if (encoded.startsWith("/"))
{
path = (encoded.length() == 1) ? "/" : URIUtil.canonicalPath(URIUtil.decodePath(encoded));
path = (encoded.length() == 1) ? "/" : URIUtil.canonicalPath(uri.getDecodedPath());
}
else if ("*".equals(encoded) || HttpMethod.CONNECT.is(getMethod()))
{

103
jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java
View File

@ -41,6 +41,7 @@ import java.util.Map;
import java.util.Set;
import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.Future;
import java.util.concurrent.atomic.AtomicReference;
import javax.servlet.DispatcherType;
import javax.servlet.Filter;
import javax.servlet.FilterRegistration;
@ -230,10 +231,14 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
public enum Availability
{
UNAVAILABLE, STARTING, AVAILABLE, SHUTDOWN,
STOPPED, // stopped and can't be made unavailable nor shutdown
STARTING, // starting inside of doStart. It may go to any of the next states.
AVAILABLE, // running normally
UNAVAILABLE, // Either a startup error or explicit call to setAvailable(false)
SHUTDOWN, // graceful shutdown
}
private volatile Availability _availability = Availability.UNAVAILABLE;
private final AtomicReference<Availability> _availability = new AtomicReference<>(Availability.STOPPED);
public ContextHandler()
{
@ -767,7 +772,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
@ManagedAttribute("true for graceful shutdown, which allows existing requests to complete")
public boolean isShutdown()
{
return _availability == Availability.SHUTDOWN;
return _availability.get() == Availability.SHUTDOWN;
}
/**
@ -777,7 +782,24 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
@Override
public Future<Void> shutdown()
{
_availability = isRunning() ? Availability.SHUTDOWN : Availability.UNAVAILABLE;
while (true)
{
Availability availability = _availability.get();
switch (availability)
{
case STOPPED:
return new FutureCallback(new IllegalStateException(getState()));
case STARTING:
case AVAILABLE:
case UNAVAILABLE:
if (!_availability.compareAndSet(availability, Availability.SHUTDOWN))
continue;
break;
default:
break;
}
break;
}
return new FutureCallback(true);
}
@ -786,7 +808,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
*/
public boolean isAvailable()
{
return _availability == Availability.AVAILABLE;
return _availability.get() == Availability.AVAILABLE;
}
/**
@ -796,12 +818,46 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
*/
public void setAvailable(boolean available)
{
synchronized (this)
// Only supported state transitions are:
// UNAVAILABLE --true---> AVAILABLE
// STARTING -----false--> UNAVAILABLE
// AVAILABLE ----false--> UNAVAILABLE
if (available)
{
if (available && isRunning())
_availability = Availability.AVAILABLE;
else if (!available || !isRunning())
_availability = Availability.UNAVAILABLE;
while (true)
{
Availability availability = _availability.get();
switch (availability)
{
case AVAILABLE:
break;
case UNAVAILABLE:
if (!_availability.compareAndSet(availability, Availability.AVAILABLE))
continue;
break;
default:
throw new IllegalStateException(availability.toString());
}
break;
}
}
else
{
while (true)
{
Availability availability = _availability.get();
switch (availability)
{
case STARTING:
case AVAILABLE:
if (!_availability.compareAndSet(availability, Availability.UNAVAILABLE))
continue;
break;
default:
break;
}
break;
}
}
}
@ -821,7 +877,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
@Override
protected void doStart() throws Exception
{
_availability = Availability.STARTING;
_availability.set(Availability.STARTING);
if (_contextPath == null)
throw new IllegalStateException("Null contextPath");
@ -856,13 +912,12 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
contextInitialized();
_availability = Availability.AVAILABLE;
_availability.compareAndSet(Availability.STARTING, Availability.AVAILABLE);
LOG.info("Started {}", this);
}
finally
{
if (_availability == Availability.STARTING)
_availability = Availability.UNAVAILABLE;
_availability.compareAndSet(Availability.STARTING, Availability.UNAVAILABLE);
exitScope(null);
__context.set(oldContext);
// reset the classloader
@ -1038,7 +1093,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
}
}
_availability = Availability.UNAVAILABLE;
_availability.set(Availability.STOPPED);
ClassLoader oldClassloader = null;
ClassLoader oldWebapploader = null;
@ -1192,8 +1247,10 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
return false;
}
switch (_availability)
switch (_availability.get())
{
case STOPPED:
return false;
case SHUTDOWN:
case UNAVAILABLE:
baseRequest.setHandled(true);
@ -1584,6 +1641,8 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
*/
public void setClassLoader(ClassLoader classLoader)
{
if (isStarted())
throw new IllegalStateException(getState());
_classLoader = classLoader;
}
@ -1814,7 +1873,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
b.append('{');
if (getDisplayName() != null)
b.append(getDisplayName()).append(',');
b.append(getContextPath()).append(',').append(getBaseResource()).append(',').append(_availability);
b.append(getContextPath()).append(',').append(getBaseResource()).append(',').append(_availability.get());
if (vhosts != null && vhosts.length > 0)
b.append(',').append(vhosts[0]);
@ -1823,7 +1882,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
return b.toString();
}
public synchronized Class<?> loadClass(String className) throws ClassNotFoundException
public Class<?> loadClass(String className) throws ClassNotFoundException
{
if (className == null)
return null;
@ -2334,7 +2393,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
* @see javax.servlet.ServletContext#getAttribute(java.lang.String)
*/
@Override
public synchronized Object getAttribute(String name)
public Object getAttribute(String name)
{
Object o = ContextHandler.this.getAttribute(name);
if (o == null)
@ -2346,7 +2405,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
* @see javax.servlet.ServletContext#getAttributeNames()
*/
@Override
public synchronized Enumeration<String> getAttributeNames()
public Enumeration<String> getAttributeNames()
{
HashSet<String> set = new HashSet<>();
Enumeration<String> e = super.getAttributeNames();
@ -2367,7 +2426,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
* @see javax.servlet.ServletContext#setAttribute(java.lang.String, java.lang.Object)
*/
@Override
public synchronized void setAttribute(String name, Object value)
public void setAttribute(String name, Object value)
{
Object oldValue = super.getAttribute(name);
@ -2396,7 +2455,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
* @see javax.servlet.ServletContext#removeAttribute(java.lang.String)
*/
@Override
public synchronized void removeAttribute(String name)
public void removeAttribute(String name)
{
Object oldValue = super.getAttribute(name);
super.removeAttribute(name);

12
jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerTest.java
View File

@ -417,6 +417,18 @@ public class ContextHandlerTest
assertThat(connector.getResponse("GET /foo/xxx HTTP/1.0\n\n"), Matchers.containsString("ctx='/foo'"));
assertThat(connector.getResponse("GET /foo/bar/xxx HTTP/1.0\n\n"), Matchers.containsString("ctx='/foo/bar'"));
// If we make foobar unavailable, then requests will be handled by 503
foobar.setAvailable(false);
assertThat(connector.getResponse("GET / HTTP/1.0\n\n"), Matchers.containsString("ctx=''"));
assertThat(connector.getResponse("GET /foo/xxx HTTP/1.0\n\n"), Matchers.containsString("ctx='/foo'"));
assertThat(connector.getResponse("GET /foo/bar/xxx HTTP/1.0\n\n"), Matchers.containsString(" 503 "));
// If we make foobar available, then requests will be handled normally
foobar.setAvailable(true);
assertThat(connector.getResponse("GET / HTTP/1.0\n\n"), Matchers.containsString("ctx=''"));
assertThat(connector.getResponse("GET /foo/xxx HTTP/1.0\n\n"), Matchers.containsString("ctx='/foo'"));
assertThat(connector.getResponse("GET /foo/bar/xxx HTTP/1.0\n\n"), Matchers.containsString("ctx='/foo/bar'"));
// If we stop foobar, then requests will be handled by foo
foobar.stop();
assertThat(connector.getResponse("GET / HTTP/1.0\n\n"), Matchers.containsString("ctx=''"));

7
jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilCanonicalPathTest.java
View File

@ -114,6 +114,13 @@ public class URIUtilCanonicalPathTest
// paths with encoded segments should remain encoded
// canonicalPath() is not responsible for decoding characters
{"%2e%2e/", "%2e%2e/"},
{"/%2e%2e/", "/%2e%2e/"},
// paths with parameters are not elided
// canonicalPath() is not responsible for decoding characters
{"/foo/.;/bar", "/foo/.;/bar"},
{"/foo/..;/bar", "/foo/..;/bar"},
{"/foo/..;/..;/bar", "/foo/..;/..;/bar"},
};
ArrayList<Arguments> ret = new ArrayList<>();

1
jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppContext.java
View File

@ -1434,7 +1434,6 @@ public class WebAppContext extends ServletContextHandler implements WebAppClassL
setClassLoader(null);
}
setAvailable(true);
_unavailableException = null;
}
}