jetty-9 fixed context restart for constraint test

jetty-security/src/main/java/org/eclipse/jetty/security/ConstraintSecurityHandler.java

@@ -236,10 +236,8 @@ public class ConstraintSecurityHandler extends SecurityHandler implements Constr
     @Override
     protected void doStop() throws Exception
     {
-        _constraintMap.clear();
-        _constraintMappings.clear();
-        _roles.clear();
         super.doStop();
+        _constraintMap.clear();
     }
 
     protected void processConstraintMapping(ConstraintMapping mapping)

jetty-security/src/main/java/org/eclipse/jetty/security/authentication/LoginAuthenticator.java

@@ -74,17 +74,20 @@ public abstract class LoginAuthenticator implements Authenticator
     {
         HttpSession httpSession = request.getSession(false);
 
-        synchronized (httpSession)
+        if (_renewSession && httpSession!=null)
         {
-            //if we should renew sessions, and there is an existing session that may have been seen by non-authenticated users
+            synchronized (httpSession)
-            //(indicated by SESSION_SECURED not being set on the session) then we should change id
-            if (_renewSession && httpSession!=null && httpSession.getAttribute(AbstractSessionManager.SESSION_KNOWN_ONLY_TO_AUTHENTICATED)!=Boolean.TRUE)
             {
-                HttpSession newSession = AbstractSessionManager.renewSession(request, httpSession,true);
+                //if we should renew sessions, and there is an existing session that may have been seen by non-authenticated users
-                LOG.debug("renew {}->{}",httpSession.getId(),newSession.getId());
+                //(indicated by SESSION_SECURED not being set on the session) then we should change id
-                httpSession=newSession;
+                if (httpSession.getAttribute(AbstractSessionManager.SESSION_KNOWN_ONLY_TO_AUTHENTICATED)!=Boolean.TRUE)
+                {
+                    HttpSession newSession = AbstractSessionManager.renewSession(request, httpSession,true);
+                    LOG.debug("renew {}->{}",httpSession.getId(),newSession.getId());
+                    httpSession=newSession;
+                }
             }
-            return httpSession;
         }
+        return httpSession;
     }
 }

jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java

@@ -305,10 +305,6 @@ public class ConstraintTest
     @Test
     public void testFormRedirect() throws Exception
     {
-        Log.getLogger(SecurityHandler.class).setDebugEnabled(true);
-        Log.getLogger(LoginAuthenticator.class).setDebugEnabled(true);
-        Log.getLogger(FormAuthenticator.class).setDebugEnabled(true);
-        
         _security.setAuthenticator(new FormAuthenticator("/testLoginPage","/testErrorPage",false));
         _security.setStrict(false);
         _server.start();
@@ -783,9 +779,8 @@ public class ConstraintTest
         _security.setHandler(check);
         _security.setAuthenticator(new BasicAuthenticator());
         _security.setStrict(false);
-        _server.start();
         
-        System.out.println(_server.dump());
+        _server.start();
 
         String response;
         response = _connector.getResponses("GET /ctx/noauth/info HTTP/1.0\r\n\r\n", 100000, TimeUnit.MILLISECONDS);
@@ -798,40 +793,12 @@ public class ConstraintTest
 
         _server.stop();
         
-        /*
-         * FIXME: this seems to indicate there is an issue with the way the server is stopping and starting now
-         * 
-         * Note that ConstraintSecurityHandler loses all of its brains when the server starts and stops, but that
-         * change was made in 2/2011 and this wasn't exposed til now, which seems to indicate that previously
-         * when the server stopped that doStop() didn't make it down to the constraint handler...and now it does.
-         * 
-         * also, seems to be an issue in local connector, I had to add a new one for it to be able to work here as well
-         * so issues in stop/start there as well
-         */
-        
-        _connector = new LocalConnector(_server);
-        _server.setConnectors(new Connector[]{_connector});
-        ContextHandler _context = new ContextHandler();
-        SessionHandler _session = new SessionHandler();
-
-        _context.setContextPath("/ctx");
-        _server.setHandler(_context);
-        _context.setHandler(_session);
-
-        _security = new ConstraintSecurityHandler();
-        _session.setHandler(_security);
-        RequestHandler _handler = new RequestHandler();
-        _security.setHandler(_handler);             
-                
         RoleRefHandler roleref = new RoleRefHandler();
+        roleref.setHandler(_security.getHandler());
         _security.setHandler(roleref);
         roleref.setHandler(check);
-       
-        _security.setConstraintMappings(getConstraintMappings(),getKnownRoles());
-        
-        _server.start();
 
-        System.out.println(_server.dump());
+        _server.start();
         
         response = _connector.getResponses("GET /ctx/auth/info HTTP/1.0\r\n" +
                 "Authorization: Basic " + B64Code.encode("user2:password") + "\r\n" +
@@ -885,6 +852,7 @@ public class ConstraintTest
     }
     private class RequestHandler extends AbstractHandler
     {
+        @Override
         public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response ) throws IOException, ServletException
         {
             baseRequest.setHandled(true);
@@ -916,16 +884,19 @@ public class ConstraintTest
 
             UserIdentity.Scope scope = new UserIdentity.Scope()
             {
+                @Override
                 public String getContextPath()
                 {
                     return "/";
                 }
 
+                @Override
                 public String getName()
                 {
                     return "someServlet";
                 }
 
+                @Override
                 public Map<String, String> getRoleRefMap()
                 {
                     Map<String, String> map = new HashMap<>();
@@ -949,6 +920,7 @@ public class ConstraintTest
 
     private class RoleCheckHandler extends AbstractHandler
     {
+        @Override
         public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response ) throws IOException, ServletException
         {
             ((Request) request).setHandled(true);

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

@@ -164,13 +164,9 @@ public class ContextHandler extends ScopedHandler implements Attributes, Server.
     private Object _requestAttributeListeners;
     private Map<String, Object> _managedAttributes;
     private String[] _protectedTargets;
-
+   
-    private boolean _shutdown = false;
+    public enum Availability { AVAILABLE,SHUTDOWN,UNAVAILABLE};
-
+    private volatile Availability _availability; 
-    private boolean _available = true;
-    private volatile int _availability; // 0=STOPPED, 1=AVAILABLE, 2=SHUTDOWN, 3=UNAVAILABLE
-
-    private final static int __STOPPED = 0, __AVAILABLE = 1, __SHUTDOWN = 2, __UNAVAILABLE = 3;
 
     /* ------------------------------------------------------------ */
     /**
@@ -629,12 +625,15 @@ public class ContextHandler extends ScopedHandler implements Attributes, Server.
     /**
      * @return true if this context is accepting new requests
      */
-    @ManagedAttribute("false if this context is accepting new requests. true for graceful shutdown, which allows existing requests to complete")
+    @ManagedAttribute("true for graceful shutdown, which allows existing requests to complete")
     public boolean isShutdown()
     {
-        synchronized (this)
+        switch(_availability)
         {
-            return !_shutdown;
+            case SHUTDOWN:
+                return true;
+            default:
+                return false;
         }
     }
 
@@ -644,13 +643,10 @@ public class ContextHandler extends ScopedHandler implements Attributes, Server.
      * requests can complete, but no new requests are accepted.
      *
      */
+    @Override
     public void shutdown()
     {
-        synchronized (this)
+        _availability = isRunning() ? Availability.SHUTDOWN : Availability.UNAVAILABLE;
-        {
-            _shutdown = true;
-            _availability = isRunning() ? __SHUTDOWN : __STOPPED;
-        }
     }
 
     /* ------------------------------------------------------------ */
@@ -659,10 +655,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Server.
      */
     public boolean isAvailable()
     {
-        synchronized (this)
+        return _availability==Availability.AVAILABLE;
-        {
-            return _available;
-        }
     }
 
     /* ------------------------------------------------------------ */
@@ -673,8 +666,10 @@ public class ContextHandler extends ScopedHandler implements Attributes, Server.
     {
         synchronized (this)
         {
-            _available = available;
+            if (available && isRunning())
-            _availability = isRunning()?(_shutdown?__SHUTDOWN:_available?__AVAILABLE:__UNAVAILABLE):__STOPPED;
+                _availability = Availability.AVAILABLE;
+            else if (!available || !isRunning())
+                _availability = Availability.UNAVAILABLE;
         }
     }
 
@@ -697,7 +692,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Server.
     @Override
     protected void doStart() throws Exception
     {
-        _availability = __STOPPED;
+        _availability = Availability.UNAVAILABLE;
 
         if (_contextPath == null)
             throw new IllegalStateException("Null contextPath");
@@ -726,10 +721,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Server.
             // defers the calling of super.doStart()
             startContext();
 
-            synchronized(this)
+            _availability = Availability.AVAILABLE;
-            {
-                _availability = _shutdown?__SHUTDOWN:_available?__AVAILABLE:__UNAVAILABLE;
-            }
         }
         finally
         {
@@ -806,7 +798,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Server.
     @Override
     protected void doStop() throws Exception
     {
-        _availability = __STOPPED;
+        _availability = Availability.UNAVAILABLE;
 
         ClassLoader old_classloader = null;
         Thread current_thread = null;
@@ -867,10 +859,8 @@ public class ContextHandler extends ScopedHandler implements Attributes, Server.
 
         switch (_availability)
         {
-            case __STOPPED:
+            case UNAVAILABLE:
-            case __SHUTDOWN:
+            case SHUTDOWN:
-                return false;
-            case __UNAVAILABLE:
                 baseRequest.setHandled(true);
                 response.sendError(HttpServletResponse.SC_SERVICE_UNAVAILABLE);
                 return false;
@@ -1517,8 +1507,8 @@ public class ContextHandler extends ScopedHandler implements Attributes, Server.
                     b.append(s.charAt(0)).append('.');
             }
         }
-        b.append(getClass().getSimpleName());
+        b.append(getClass().getSimpleName()).append('@').append(Integer.toString(hashCode(),16));
-        b.append('{').append(getContextPath()).append(',').append(getBaseResource());
+        b.append('{').append(getContextPath()).append(',').append(getBaseResource()).append(',').append(_availability);
 
         if (vhosts != null && vhosts.length > 0)
             b.append(',').append(vhosts[0]);

jetty-server/src/test/java/org/eclipse/jetty/server/LocalConnectorTest.java

@@ -58,6 +58,21 @@ public class LocalConnectorTest
         assertThat(response,containsString("pathInfo=/R1"));
     }
     
+    @Test
+    public void testStopStart() throws Exception
+    {        
+        String response=_connector.getResponses("GET /R1 HTTP/1.0\r\n\r\n");
+        assertThat(response,containsString("HTTP/1.1 200 OK"));
+        assertThat(response,containsString("pathInfo=/R1"));
+        
+        _server.stop();
+        _server.start(); 
+        
+        response=_connector.getResponses("GET /R2 HTTP/1.0\r\n\r\n");
+        assertThat(response,containsString("HTTP/1.1 200 OK"));
+        assertThat(response,containsString("pathInfo=/R2"));
+    }
+    
     @Test
     public void testTwoGETs() throws Exception
     {