[UNDERTOW-2069] Fix the deadlock and add a test for it

servlet/src/main/java/io/undertow/servlet/core/ManagedFilter.java

@@ -19,6 +19,7 @@
 package io.undertow.servlet.core;
 
 import java.io.IOException;
+import java.util.concurrent.atomic.AtomicIntegerFieldUpdater;
 
 import jakarta.servlet.Filter;
 import jakarta.servlet.FilterChain;
@@ -34,6 +35,8 @@
 import io.undertow.servlet.spec.FilterConfigImpl;
 import io.undertow.servlet.spec.ServletContextImpl;
 
+import static org.xnio.Bits.anyAreSet;
+
 /**
  * @author Stuart Douglas
  */
@@ -42,7 +45,12 @@ public class ManagedFilter implements Lifecycle {
     private final FilterInfo filterInfo;
     private final ServletContextImpl servletContext;
 
-    private volatile boolean started = false;
+    private static final int FLAG_STARTED = 1;
+    private static final int FLAG_STOPPED = 1 << 1;
+    @SuppressWarnings("unused")
+    private volatile int state;
+    private static final AtomicIntegerFieldUpdater<ManagedFilter> stateFieldUpdater = AtomicIntegerFieldUpdater.newUpdater(ManagedFilter.class, "state");
+
     private volatile Filter filter;
     private volatile InstanceHandle<? extends Filter> handle;
 
@@ -55,9 +63,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
         if(servletContext.getDeployment().getDeploymentState() != DeploymentManager.State.STARTED) {
             throw UndertowServletMessages.MESSAGES.deploymentStopped(servletContext.getDeployment().getDeploymentInfo().getDeploymentName());
         }
-        if (!started) {
-            start();
-        }
+        start();
         getFilter().doFilter(request, response, chain);
     }
 
@@ -83,30 +89,33 @@ public void createFilter() throws ServletException {
         }
     }
 
-    public synchronized void start() throws ServletException {
-        if (!started) {
-
-            started = true;
-        }
+    public void start() throws ServletException {
+        do {
+            if (anyAreSet(stateFieldUpdater.get(this), FLAG_STOPPED)) {
+                throw UndertowServletMessages.MESSAGES.deploymentStopped(servletContext.getDeployment().getDeploymentInfo().getDeploymentName());
+            }
+        } while (stateFieldUpdater.get(this) != FLAG_STARTED && !stateFieldUpdater.compareAndSet(this, 0, FLAG_STARTED));
     }
 
-    public synchronized void stop() {
-        started = false;
-        if (handle != null) {
-            try {
-                new LifecyleInterceptorInvocation(servletContext.getDeployment().getDeploymentInfo().getLifecycleInterceptors(), filterInfo, filter).proceed();
-            } catch (Exception e) {
-                UndertowServletLogger.ROOT_LOGGER.failedToDestroy(filterInfo, e);
+    public void stop() {
+        stateFieldUpdater.set(this, FLAG_STOPPED);
+        synchronized (this) {
+            if (handle != null) {
+                try {
+                    new LifecyleInterceptorInvocation(servletContext.getDeployment().getDeploymentInfo().getLifecycleInterceptors(), filterInfo, filter).proceed();
+                } catch (Exception e) {
+                    UndertowServletLogger.ROOT_LOGGER.failedToDestroy(filterInfo, e);
+                }
+                handle.release();
             }
-            handle.release();
+            filter = null;
+            handle = null;
         }
-        filter = null;
-        handle = null;
     }
 
     @Override
     public boolean isStarted() {
-        return started;
+        return anyAreSet(state, FLAG_STARTED);
     }
 
     public FilterInfo getFilterInfo() {

servlet/src/test/java/io/undertow/servlet/test/dispatchingfilter/DispatchingFilter.java

@@ -0,0 +1,71 @@
+/*
+ * JBoss, Home of Professional Open Source.
+ * Copyright 2022 Red Hat, Inc., and individual contributors
+ * as indicated by the @author tags.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License.
+ */
+
+package io.undertow.servlet.test.dispatchingfilter;
+
+import jakarta.servlet.DispatcherType;
+import jakarta.servlet.Filter;
+import jakarta.servlet.FilterChain;
+import jakarta.servlet.FilterConfig;
+import jakarta.servlet.RequestDispatcher;
+import jakarta.servlet.ServletException;
+import jakarta.servlet.ServletRequest;
+import jakarta.servlet.ServletResponse;
+
+import java.io.IOException;
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+
+/**
+ * Dispatching filter whose doFilter and destroy methods share a lock.
+ *
+ * @author Flavia Rainone
+ */
+public class DispatchingFilter implements Filter {
+
+    static ReadWriteLock rwLock = new ReentrantReadWriteLock();
+
+    @Override
+    public void init(FilterConfig filterConfig) throws ServletException {
+
+    }
+
+    @Override
+    public void doFilter(ServletRequest req,
+                         ServletResponse res,
+                         FilterChain chain) throws IOException, ServletException {
+        rwLock.writeLock().lock();
+        try {
+            if (req.getDispatcherType() != DispatcherType.FORWARD) {
+                // race condition test semaphore: flag to test when the deployment should be stopped
+                DispatchingFilterTestCase.readyToStop.setResult(true);
+                RequestDispatcher dispatcher = req.getServletContext().getRequestDispatcher("/path/servlet");
+                dispatcher.forward(req, res);
+            }
+            chain.doFilter(req, res);
+        } finally {
+            rwLock.writeLock().unlock();
+        }
+    }
+
+    @Override
+    public void destroy() {
+        rwLock.writeLock().lock();
+        rwLock.writeLock().unlock();
+    }
+}

servlet/src/test/java/io/undertow/servlet/test/dispatchingfilter/DispatchingFilterTestCase.java

@@ -0,0 +1,106 @@
+/*
+ * JBoss, Home of Professional Open Source.
+ * Copyright 2022 Red Hat, Inc., and individual contributors
+ * as indicated by the @author tags.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License.
+ */
+
+package io.undertow.servlet.test.dispatchingfilter;
+
+import io.undertow.server.handlers.PathHandler;
+import io.undertow.servlet.api.DeploymentInfo;
+import io.undertow.servlet.api.DeploymentManager;
+import io.undertow.servlet.api.FilterInfo;
+import io.undertow.servlet.api.ServletContainer;
+import io.undertow.servlet.api.ServletInfo;
+import io.undertow.servlet.test.path.ServletPathMappingTestCase;
+import io.undertow.servlet.test.util.PathTestServlet;
+import io.undertow.servlet.test.util.TestClassIntrospector;
+import io.undertow.testutils.DefaultServer;
+import io.undertow.testutils.TestHttpClient;
+import io.undertow.util.StatusCodes;
+import jakarta.servlet.DispatcherType;
+import jakarta.servlet.ServletException;
+import org.apache.http.HttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.junit.Assert;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.xnio.FutureResult;
+
+import java.io.IOException;
+
+/**
+ * Tests if a dispatching filter with a lock used by both doFilter and destroy can be safely destroyed during filtering.
+ * See UNDERTOW-2069.
+ *
+ * @author Flavia Rainone
+ */
+@RunWith(DefaultServer.class)
+public class DispatchingFilterTestCase {
+
+    static FutureResult<Boolean> readyToStop = new FutureResult();
+
+    private static DeploymentManager manager;
+
+    @BeforeClass
+    public static void setup() throws ServletException {
+
+        final PathHandler root = new PathHandler();
+        final ServletContainer container = ServletContainer.Factory.newInstance();
+
+        DeploymentInfo builder = new DeploymentInfo()
+                .setClassIntrospecter(TestClassIntrospector.INSTANCE)
+                .setClassLoader(ServletPathMappingTestCase.class.getClassLoader())
+                .setContextPath("/servletContext")
+                .setDeploymentName("servletContext.war");
+
+        builder.addServlet(new ServletInfo("DefaultTestServlet", PathTestServlet.class)
+                .addMapping("/path/default"));
+
+        builder.addFilter(new FilterInfo("dispatching-filter", DispatchingFilter.class));
+        builder.addFilterUrlMapping("dispatching-filter", "/*", DispatcherType.REQUEST);
+        builder.addFilterUrlMapping("dispatching-filter", "/*", DispatcherType.FORWARD);
+
+        manager = container.addDeployment(builder);
+        manager.deploy();
+        root.addPrefixPath(builder.getContextPath(), manager.start());
+        DefaultServer.setRootHandler(root);
+    }
+
+    @Test
+    public void test() throws InterruptedException, ServletException, IOException {
+        final FutureResult<HttpResponse> responseFuture = new FutureResult<>();
+        new Thread(() -> {
+            TestHttpClient client = new TestHttpClient();
+            try {
+                HttpGet get = new HttpGet(DefaultServer.getDefaultServerURL() + "/servletContext/path/default");
+                responseFuture.setResult(client.execute(get));
+            } catch (org.apache.http.client.ClientProtocolException e) {
+                e.printStackTrace();
+            } catch (IOException e) {
+                responseFuture.setException(e);
+            } finally {
+                client.getConnectionManager().shutdown();
+            }
+        }).start();
+        readyToStop.getIoFuture().await();
+        manager.stop();
+        final HttpResponse result = responseFuture.getIoFuture().get();
+        final int statusCode = result.getStatusLine().getStatusCode();
+        // the deployment is stopped, we can expect an 500 or 404 (depends on OS)
+        Assert.assertTrue(statusCode == StatusCodes.INTERNAL_SERVER_ERROR || statusCode == io.undertow.util.StatusCodes.NOT_FOUND);
+    }
+}