From ec2ad2099e8cbf6d9a1874346bab6a5b017161b0 Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Mon, 25 Nov 2019 11:57:19 +1100 Subject: [PATCH 1/6] Issue #4351 Lazy servlet init is not atomic Signed-off-by: Jan Bartel --- .../eclipse/jetty/servlet/ServletHolder.java | 17 +- .../jetty/servlet/InitServletTest.java | 152 ++++++++++++++++++ 2 files changed, 158 insertions(+), 11 deletions(-) create mode 100644 jetty-servlet/src/test/java/org/eclipse/jetty/servlet/InitServletTest.java diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java index db8a0295ab04..a344566a4532 100644 --- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java +++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java @@ -470,21 +470,16 @@ public void destroyInstance(Object o) public Servlet getServlet() throws ServletException { - Servlet servlet = _servlet; - if (servlet == null) + synchronized (this) { - synchronized (this) + if (_servlet == null && isRunning()) { - servlet = _servlet; - if (servlet == null && isRunning()) - { - if (getHeldClass() != null) - initServlet(); - servlet = _servlet; - } + if (getHeldClass() != null) + initServlet(); } } - return servlet; + + return _servlet; } /** diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/InitServletTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/InitServletTest.java new file mode 100644 index 000000000000..d9bd87e84509 --- /dev/null +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/InitServletTest.java @@ -0,0 +1,152 @@ +// +// ======================================================================== +// Copyright (c) 1995-2019 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.servlet; + +import java.io.IOException; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; + +import javax.servlet.ServletException; +import javax.servlet.http.HttpServlet; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import org.eclipse.jetty.client.HttpClient; +import org.eclipse.jetty.client.api.Request; +import org.eclipse.jetty.client.api.Response; +import org.eclipse.jetty.client.api.Result; +import org.eclipse.jetty.http.HttpMethod; +import org.eclipse.jetty.http.HttpStatus; +import org.eclipse.jetty.server.NetworkConnector; +import org.eclipse.jetty.server.Server; +import org.hamcrest.Matchers; +import org.junit.jupiter.api.Test; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.array; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class InitServletTest +{ + + public static class DemoServlet extends HttpServlet + { + AtomicInteger initCount = new AtomicInteger(); + + @Override + public void init() throws ServletException + { + super.init(); + try + { + //Make the initialization last a little while so + //other request can run + Thread.sleep(5000); + } + catch (InterruptedException e) + { + throw new ServletException(e); + } + //check the servlet hasn't been initialized before + int val = initCount.addAndGet(1); + assertThat(val, Matchers.equalTo(1)); + } + + @Override + public void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException + { + if (initCount.get() != 1) + { + resp.sendError(500, "Servlet not initialized!"); + } + } + } + + private static class AsyncResponseListener implements Response.CompleteListener + { + private CountDownLatch resultsLatch; + private Integer[] results; + private AtomicInteger index = new AtomicInteger(); + + public AsyncResponseListener(int count) + { + resultsLatch = new CountDownLatch(count); + results = new Integer[count]; + } + + public void onComplete(Result result) + { + results[index.getAndAdd(1)] = result.getResponse().getStatus(); + resultsLatch.countDown(); + } + + public void awaitCompletion() throws InterruptedException + { + assertTrue(resultsLatch.await(60L, TimeUnit.SECONDS)); + } + + public Integer[] getResults() + { + return results; + } + } + + @Test + public void testServletInitialization() throws Exception + { + Server server = new Server(0); + ServletContextHandler context = new ServletContextHandler(server, "/"); + server.setHandler(context); + //add a lazily instantiated servlet + context.addServlet(new ServletHolder(DemoServlet.class), "/*"); + server.start(); + final int port = ((NetworkConnector)server.getConnectors()[0]).getLocalPort(); + HttpClient client = new HttpClient(); + try + { + client.start(); + + //Expect 2 responses + final AsyncResponseListener l = new AsyncResponseListener(2); + + //req1 + Request r1 = client.newRequest("http://localhost:" + port + "/r1"); + r1.method(HttpMethod.GET).send(l); + + //Need to give 1st request a head start before request2 + Thread.sleep(1000); + + //req2 + Request r2 = client.newRequest("http://localhost:" + port + "/r2"); + r2.method(HttpMethod.GET).send(l); + + l.awaitCompletion(); + + assertThat(l.getResults(), is(array(equalTo(HttpStatus.OK_200), equalTo(HttpStatus.OK_200)))); + } + finally + { + client.stop(); + server.stop(); + } + } +} From f0a3d465ec0d8311e4bfc16164a7f83cb712b77a Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Tue, 26 Nov 2019 08:43:11 +1100 Subject: [PATCH 2/6] Issue #4351 Changes post review. Signed-off-by: Jan Bartel --- .../eclipse/jetty/servlet/ServletHolder.java | 2 +- .../jetty/servlet/InitServletTest.java | 31 +++++++++---------- 2 files changed, 15 insertions(+), 18 deletions(-) diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java index a344566a4532..decacb58fdf3 100644 --- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java +++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java @@ -621,7 +621,7 @@ protected void initJspServlet() throws Exception ContextHandler ch = ContextHandler.getContextHandler(getServletHandler().getServletContext()); /* Set the webapp's classpath for Jasper */ - ch.setAttribute("org.apache.catalina.jspgetHeldClass()path", ch.getClassPath()); + ch.setAttribute("org.apache.catalina.jsp_classpath", ch.getClassPath()); /* Set up other classpath attribute */ if ("?".equals(getInitParameter("classpath"))) diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/InitServletTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/InitServletTest.java index d9bd87e84509..66362223a778 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/InitServletTest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/InitServletTest.java @@ -47,7 +47,6 @@ public class InitServletTest { - public static class DemoServlet extends HttpServlet { AtomicInteger initCount = new AtomicInteger(); @@ -66,14 +65,14 @@ public void init() throws ServletException { throw new ServletException(e); } - //check the servlet hasn't been initialized before - int val = initCount.addAndGet(1); - assertThat(val, Matchers.equalTo(1)); + initCount.addAndGet(1); } @Override public void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException { + //Check that the init() method has been totally finished (by another request) before + //the servlet service() method is called. if (initCount.get() != 1) { resp.sendError(500, "Servlet not initialized!"); @@ -83,14 +82,15 @@ public void doGet(HttpServletRequest req, HttpServletResponse resp) throws Servl private static class AsyncResponseListener implements Response.CompleteListener { + private CountDownLatch resultsLatch; private Integer[] results; private AtomicInteger index = new AtomicInteger(); - public AsyncResponseListener(int count) + public AsyncResponseListener(CountDownLatch resultsLatch, Integer[] results) { - resultsLatch = new CountDownLatch(count); - results = new Integer[count]; + this.resultsLatch = resultsLatch; + this.results = results; } public void onComplete(Result result) @@ -103,11 +103,6 @@ public void awaitCompletion() throws InterruptedException { assertTrue(resultsLatch.await(60L, TimeUnit.SECONDS)); } - - public Integer[] getResults() - { - return results; - } } @Test @@ -119,29 +114,31 @@ public void testServletInitialization() throws Exception //add a lazily instantiated servlet context.addServlet(new ServletHolder(DemoServlet.class), "/*"); server.start(); - final int port = ((NetworkConnector)server.getConnectors()[0]).getLocalPort(); + int port = ((NetworkConnector)server.getConnectors()[0]).getLocalPort(); HttpClient client = new HttpClient(); try { client.start(); //Expect 2 responses - final AsyncResponseListener l = new AsyncResponseListener(2); + CountDownLatch resultsLatch = new CountDownLatch(2); + Integer[] results = new Integer[2]; + AsyncResponseListener l = new AsyncResponseListener(resultsLatch, results); - //req1 + //req1: should initialize servlet Request r1 = client.newRequest("http://localhost:" + port + "/r1"); r1.method(HttpMethod.GET).send(l); //Need to give 1st request a head start before request2 Thread.sleep(1000); - //req2 + //req2: should see servlet fully initialized by request1 Request r2 = client.newRequest("http://localhost:" + port + "/r2"); r2.method(HttpMethod.GET).send(l); l.awaitCompletion(); - assertThat(l.getResults(), is(array(equalTo(HttpStatus.OK_200), equalTo(HttpStatus.OK_200)))); + assertThat(results, is(array(equalTo(HttpStatus.OK_200), equalTo(HttpStatus.OK_200)))); } finally { From 5bc6593337893a6e78f56361ac029964e43a9b8c Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Tue, 26 Nov 2019 11:03:41 +1100 Subject: [PATCH 3/6] Issue #4351 Found simpler test Signed-off-by: Jan Bartel --- .../jetty/servlet/InitServletTest.java | 149 ------------------ .../jetty/servlet/ServletHolderTest.java | 89 +++++++++++ 2 files changed, 89 insertions(+), 149 deletions(-) delete mode 100644 jetty-servlet/src/test/java/org/eclipse/jetty/servlet/InitServletTest.java diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/InitServletTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/InitServletTest.java deleted file mode 100644 index 66362223a778..000000000000 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/InitServletTest.java +++ /dev/null @@ -1,149 +0,0 @@ -// -// ======================================================================== -// Copyright (c) 1995-2019 Mort Bay Consulting Pty. Ltd. -// ------------------------------------------------------------------------ -// All rights reserved. This program and the accompanying materials -// are made available under the terms of the Eclipse Public License v1.0 -// and Apache License v2.0 which accompanies this distribution. -// -// The Eclipse Public License is available at -// http://www.eclipse.org/legal/epl-v10.html -// -// The Apache License v2.0 is available at -// http://www.opensource.org/licenses/apache2.0.php -// -// You may elect to redistribute this code under either of these licenses. -// ======================================================================== -// - -package org.eclipse.jetty.servlet; - -import java.io.IOException; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicInteger; - -import javax.servlet.ServletException; -import javax.servlet.http.HttpServlet; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; - -import org.eclipse.jetty.client.HttpClient; -import org.eclipse.jetty.client.api.Request; -import org.eclipse.jetty.client.api.Response; -import org.eclipse.jetty.client.api.Result; -import org.eclipse.jetty.http.HttpMethod; -import org.eclipse.jetty.http.HttpStatus; -import org.eclipse.jetty.server.NetworkConnector; -import org.eclipse.jetty.server.Server; -import org.hamcrest.Matchers; -import org.junit.jupiter.api.Test; - -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.array; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.is; -import static org.junit.jupiter.api.Assertions.assertTrue; - -public class InitServletTest -{ - public static class DemoServlet extends HttpServlet - { - AtomicInteger initCount = new AtomicInteger(); - - @Override - public void init() throws ServletException - { - super.init(); - try - { - //Make the initialization last a little while so - //other request can run - Thread.sleep(5000); - } - catch (InterruptedException e) - { - throw new ServletException(e); - } - initCount.addAndGet(1); - } - - @Override - public void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException - { - //Check that the init() method has been totally finished (by another request) before - //the servlet service() method is called. - if (initCount.get() != 1) - { - resp.sendError(500, "Servlet not initialized!"); - } - } - } - - private static class AsyncResponseListener implements Response.CompleteListener - { - - private CountDownLatch resultsLatch; - private Integer[] results; - private AtomicInteger index = new AtomicInteger(); - - public AsyncResponseListener(CountDownLatch resultsLatch, Integer[] results) - { - this.resultsLatch = resultsLatch; - this.results = results; - } - - public void onComplete(Result result) - { - results[index.getAndAdd(1)] = result.getResponse().getStatus(); - resultsLatch.countDown(); - } - - public void awaitCompletion() throws InterruptedException - { - assertTrue(resultsLatch.await(60L, TimeUnit.SECONDS)); - } - } - - @Test - public void testServletInitialization() throws Exception - { - Server server = new Server(0); - ServletContextHandler context = new ServletContextHandler(server, "/"); - server.setHandler(context); - //add a lazily instantiated servlet - context.addServlet(new ServletHolder(DemoServlet.class), "/*"); - server.start(); - int port = ((NetworkConnector)server.getConnectors()[0]).getLocalPort(); - HttpClient client = new HttpClient(); - try - { - client.start(); - - //Expect 2 responses - CountDownLatch resultsLatch = new CountDownLatch(2); - Integer[] results = new Integer[2]; - AsyncResponseListener l = new AsyncResponseListener(resultsLatch, results); - - //req1: should initialize servlet - Request r1 = client.newRequest("http://localhost:" + port + "/r1"); - r1.method(HttpMethod.GET).send(l); - - //Need to give 1st request a head start before request2 - Thread.sleep(1000); - - //req2: should see servlet fully initialized by request1 - Request r2 = client.newRequest("http://localhost:" + port + "/r2"); - r2.method(HttpMethod.GET).send(l); - - l.awaitCompletion(); - - assertThat(results, is(array(equalTo(HttpStatus.OK_200), equalTo(HttpStatus.OK_200)))); - } - finally - { - client.stop(); - server.stop(); - } - } -} diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletHolderTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletHolderTest.java index c2c5b43dc6ef..8b0abaec424d 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletHolderTest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletHolderTest.java @@ -20,10 +20,16 @@ import java.util.Collections; import java.util.Set; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; + +import javax.servlet.ServletException; import javax.servlet.ServletRegistration; import javax.servlet.UnavailableException; import javax.servlet.http.HttpServlet; +import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.handler.ContextHandler; import org.eclipse.jetty.util.MultiException; import org.eclipse.jetty.util.log.StacklessLogging; @@ -42,6 +48,89 @@ public class ServletHolderTest { public static class FakeServlet extends HttpServlet { + public AtomicInteger initCount = new AtomicInteger(); + + @Override + public void init() throws ServletException + { + super.init(); + try + { + //Make the initialization last a little while so + //other request can run + Thread.sleep(1000); + } + catch (InterruptedException e) + { + throw new ServletException(e); + } + initCount.addAndGet(1); + } + } + + @Test + public void testServletInitAtomic() throws Exception + { + Server server = new Server(); + ServletContextHandler context = new ServletContextHandler(); + final ServletHolder holder = new ServletHolder(Source.EMBEDDED); + holder.setHeldClass(FakeServlet.class); + holder.setName("Fake"); + context.addServlet(holder, "/fake/*"); + server.setHandler(context); + server.start(); + + final AtomicInteger t1Count = new AtomicInteger(); + final AtomicInteger t2Count = new AtomicInteger(); + final CountDownLatch t1Latch = new CountDownLatch(1); + final CountDownLatch t2Latch = new CountDownLatch(1); + + //Test that 2 calls to holder.getServlet are atomic - only + //one should call servlet.init() and the other should be + //held waiting until that fully finishes. + Thread t1 = new Thread() + { + public void run() + { + try + { + FakeServlet s = (FakeServlet)holder.getServlet(); + t1Count.set(s.initCount.get()); + t1Latch.countDown(); + } + catch (ServletException e) + { + fail(e); + } + } + }; + + Thread t2 = new Thread() + { + public void run() + { + try + { + FakeServlet s = (FakeServlet)holder.getServlet(); + t2Count.set(s.initCount.get()); + t2Latch.countDown(); + } + catch (ServletException e) + { + fail(e); + } + } + }; + + t1.start(); + Thread.sleep(500); + t2.start(); + + t1Latch.await(10L, TimeUnit.SECONDS); + t2Latch.await(10L, TimeUnit.SECONDS); + + assertEquals(1, t1Count.get()); + assertEquals(1, t2Count.get()); } @Test From a6cdec548ceae6924c24882c0001c44cf2547aac Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Thu, 28 Nov 2019 09:16:14 +1100 Subject: [PATCH 4/6] Revert "Issue #4351 Found simpler test" This reverts commit 5bc6593337893a6e78f56361ac029964e43a9b8c. --- .../jetty/servlet/InitServletTest.java | 149 ++++++++++++++++++ .../jetty/servlet/ServletHolderTest.java | 89 ----------- 2 files changed, 149 insertions(+), 89 deletions(-) create mode 100644 jetty-servlet/src/test/java/org/eclipse/jetty/servlet/InitServletTest.java diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/InitServletTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/InitServletTest.java new file mode 100644 index 000000000000..66362223a778 --- /dev/null +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/InitServletTest.java @@ -0,0 +1,149 @@ +// +// ======================================================================== +// Copyright (c) 1995-2019 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.servlet; + +import java.io.IOException; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; + +import javax.servlet.ServletException; +import javax.servlet.http.HttpServlet; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import org.eclipse.jetty.client.HttpClient; +import org.eclipse.jetty.client.api.Request; +import org.eclipse.jetty.client.api.Response; +import org.eclipse.jetty.client.api.Result; +import org.eclipse.jetty.http.HttpMethod; +import org.eclipse.jetty.http.HttpStatus; +import org.eclipse.jetty.server.NetworkConnector; +import org.eclipse.jetty.server.Server; +import org.hamcrest.Matchers; +import org.junit.jupiter.api.Test; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.array; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class InitServletTest +{ + public static class DemoServlet extends HttpServlet + { + AtomicInteger initCount = new AtomicInteger(); + + @Override + public void init() throws ServletException + { + super.init(); + try + { + //Make the initialization last a little while so + //other request can run + Thread.sleep(5000); + } + catch (InterruptedException e) + { + throw new ServletException(e); + } + initCount.addAndGet(1); + } + + @Override + public void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException + { + //Check that the init() method has been totally finished (by another request) before + //the servlet service() method is called. + if (initCount.get() != 1) + { + resp.sendError(500, "Servlet not initialized!"); + } + } + } + + private static class AsyncResponseListener implements Response.CompleteListener + { + + private CountDownLatch resultsLatch; + private Integer[] results; + private AtomicInteger index = new AtomicInteger(); + + public AsyncResponseListener(CountDownLatch resultsLatch, Integer[] results) + { + this.resultsLatch = resultsLatch; + this.results = results; + } + + public void onComplete(Result result) + { + results[index.getAndAdd(1)] = result.getResponse().getStatus(); + resultsLatch.countDown(); + } + + public void awaitCompletion() throws InterruptedException + { + assertTrue(resultsLatch.await(60L, TimeUnit.SECONDS)); + } + } + + @Test + public void testServletInitialization() throws Exception + { + Server server = new Server(0); + ServletContextHandler context = new ServletContextHandler(server, "/"); + server.setHandler(context); + //add a lazily instantiated servlet + context.addServlet(new ServletHolder(DemoServlet.class), "/*"); + server.start(); + int port = ((NetworkConnector)server.getConnectors()[0]).getLocalPort(); + HttpClient client = new HttpClient(); + try + { + client.start(); + + //Expect 2 responses + CountDownLatch resultsLatch = new CountDownLatch(2); + Integer[] results = new Integer[2]; + AsyncResponseListener l = new AsyncResponseListener(resultsLatch, results); + + //req1: should initialize servlet + Request r1 = client.newRequest("http://localhost:" + port + "/r1"); + r1.method(HttpMethod.GET).send(l); + + //Need to give 1st request a head start before request2 + Thread.sleep(1000); + + //req2: should see servlet fully initialized by request1 + Request r2 = client.newRequest("http://localhost:" + port + "/r2"); + r2.method(HttpMethod.GET).send(l); + + l.awaitCompletion(); + + assertThat(results, is(array(equalTo(HttpStatus.OK_200), equalTo(HttpStatus.OK_200)))); + } + finally + { + client.stop(); + server.stop(); + } + } +} diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletHolderTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletHolderTest.java index 8b0abaec424d..c2c5b43dc6ef 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletHolderTest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletHolderTest.java @@ -20,16 +20,10 @@ import java.util.Collections; import java.util.Set; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicInteger; - -import javax.servlet.ServletException; import javax.servlet.ServletRegistration; import javax.servlet.UnavailableException; import javax.servlet.http.HttpServlet; -import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.handler.ContextHandler; import org.eclipse.jetty.util.MultiException; import org.eclipse.jetty.util.log.StacklessLogging; @@ -48,89 +42,6 @@ public class ServletHolderTest { public static class FakeServlet extends HttpServlet { - public AtomicInteger initCount = new AtomicInteger(); - - @Override - public void init() throws ServletException - { - super.init(); - try - { - //Make the initialization last a little while so - //other request can run - Thread.sleep(1000); - } - catch (InterruptedException e) - { - throw new ServletException(e); - } - initCount.addAndGet(1); - } - } - - @Test - public void testServletInitAtomic() throws Exception - { - Server server = new Server(); - ServletContextHandler context = new ServletContextHandler(); - final ServletHolder holder = new ServletHolder(Source.EMBEDDED); - holder.setHeldClass(FakeServlet.class); - holder.setName("Fake"); - context.addServlet(holder, "/fake/*"); - server.setHandler(context); - server.start(); - - final AtomicInteger t1Count = new AtomicInteger(); - final AtomicInteger t2Count = new AtomicInteger(); - final CountDownLatch t1Latch = new CountDownLatch(1); - final CountDownLatch t2Latch = new CountDownLatch(1); - - //Test that 2 calls to holder.getServlet are atomic - only - //one should call servlet.init() and the other should be - //held waiting until that fully finishes. - Thread t1 = new Thread() - { - public void run() - { - try - { - FakeServlet s = (FakeServlet)holder.getServlet(); - t1Count.set(s.initCount.get()); - t1Latch.countDown(); - } - catch (ServletException e) - { - fail(e); - } - } - }; - - Thread t2 = new Thread() - { - public void run() - { - try - { - FakeServlet s = (FakeServlet)holder.getServlet(); - t2Count.set(s.initCount.get()); - t2Latch.countDown(); - } - catch (ServletException e) - { - fail(e); - } - } - }; - - t1.start(); - Thread.sleep(500); - t2.start(); - - t1Latch.await(10L, TimeUnit.SECONDS); - t2Latch.await(10L, TimeUnit.SECONDS); - - assertEquals(1, t1Count.get()); - assertEquals(1, t2Count.get()); } @Test From 07efbb728d47b8030481cce317e299187a137333 Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Thu, 28 Nov 2019 09:34:39 +1100 Subject: [PATCH 5/6] Issue #4351 Lessen wait times. Signed-off-by: Jan Bartel --- .../test/java/org/eclipse/jetty/servlet/InitServletTest.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/InitServletTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/InitServletTest.java index 66362223a778..70935978a150 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/InitServletTest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/InitServletTest.java @@ -59,7 +59,7 @@ public void init() throws ServletException { //Make the initialization last a little while so //other request can run - Thread.sleep(5000); + Thread.sleep(2000); } catch (InterruptedException e) { @@ -76,6 +76,7 @@ public void doGet(HttpServletRequest req, HttpServletResponse resp) throws Servl if (initCount.get() != 1) { resp.sendError(500, "Servlet not initialized!"); + System.err.println("NOT INIT"); } } } @@ -130,7 +131,7 @@ public void testServletInitialization() throws Exception r1.method(HttpMethod.GET).send(l); //Need to give 1st request a head start before request2 - Thread.sleep(1000); + Thread.sleep(500); //req2: should see servlet fully initialized by request1 Request r2 = client.newRequest("http://localhost:" + port + "/r2"); From dc0fac5806040d5ac63d4db5ca623c2a859720c5 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Thu, 28 Nov 2019 11:07:24 +0100 Subject: [PATCH 6/6] Issue #4351 - Lazy servlet init is not atomic. Code cleanup. Signed-off-by: Simone Bordet --- .../jetty/servlet/InitServletTest.java | 92 ++++++++----------- 1 file changed, 37 insertions(+), 55 deletions(-) diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/InitServletTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/InitServletTest.java index 70935978a150..cd780b6b4c62 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/InitServletTest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/InitServletTest.java @@ -22,34 +22,28 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; - import javax.servlet.ServletException; import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import org.eclipse.jetty.client.HttpClient; -import org.eclipse.jetty.client.api.Request; import org.eclipse.jetty.client.api.Response; import org.eclipse.jetty.client.api.Result; -import org.eclipse.jetty.http.HttpMethod; import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.server.NetworkConnector; import org.eclipse.jetty.server.Server; -import org.hamcrest.Matchers; import org.junit.jupiter.api.Test; -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.array; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.is; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; public class InitServletTest { public static class DemoServlet extends HttpServlet { - AtomicInteger initCount = new AtomicInteger(); + private static final long INIT_SLEEP = 2000; + private final AtomicInteger initCount = new AtomicInteger(); @Override public void init() throws ServletException @@ -57,53 +51,44 @@ public void init() throws ServletException super.init(); try { - //Make the initialization last a little while so - //other request can run - Thread.sleep(2000); + // Make the initialization last a little while. + // Other requests must wait. + Thread.sleep(INIT_SLEEP); } catch (InterruptedException e) { throw new ServletException(e); } - initCount.addAndGet(1); + initCount.incrementAndGet(); } @Override public void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException { - //Check that the init() method has been totally finished (by another request) before - //the servlet service() method is called. + // Check that the init() method has been totally finished (by another request) + // before the servlet service() method is called. if (initCount.get() != 1) - { resp.sendError(500, "Servlet not initialized!"); - System.err.println("NOT INIT"); - } } } - + private static class AsyncResponseListener implements Response.CompleteListener { + private final AtomicInteger index = new AtomicInteger(); + private final CountDownLatch resultsLatch; + private final int[] results; - private CountDownLatch resultsLatch; - private Integer[] results; - private AtomicInteger index = new AtomicInteger(); - - public AsyncResponseListener(CountDownLatch resultsLatch, Integer[] results) + public AsyncResponseListener(CountDownLatch resultsLatch, int[] results) { - this.resultsLatch = resultsLatch; - this.results = results; + this.resultsLatch = resultsLatch; + this.results = results; } - + public void onComplete(Result result) { - results[index.getAndAdd(1)] = result.getResponse().getStatus(); + results[index.getAndIncrement()] = result.getResponse().getStatus(); resultsLatch.countDown(); } - - public void awaitCompletion() throws InterruptedException - { - assertTrue(resultsLatch.await(60L, TimeUnit.SECONDS)); - } } @Test @@ -112,38 +97,35 @@ public void testServletInitialization() throws Exception Server server = new Server(0); ServletContextHandler context = new ServletContextHandler(server, "/"); server.setHandler(context); - //add a lazily instantiated servlet + // Add a lazily instantiated servlet. context.addServlet(new ServletHolder(DemoServlet.class), "/*"); - server.start(); - int port = ((NetworkConnector)server.getConnectors()[0]).getLocalPort(); HttpClient client = new HttpClient(); + server.addBean(client); + server.start(); try { - client.start(); + int port = ((NetworkConnector)server.getConnectors()[0]).getLocalPort(); - //Expect 2 responses + // Expect 2 responses CountDownLatch resultsLatch = new CountDownLatch(2); - Integer[] results = new Integer[2]; + int[] results = new int[2]; AsyncResponseListener l = new AsyncResponseListener(resultsLatch, results); - - //req1: should initialize servlet - Request r1 = client.newRequest("http://localhost:" + port + "/r1"); - r1.method(HttpMethod.GET).send(l); - - //Need to give 1st request a head start before request2 - Thread.sleep(500); - - //req2: should see servlet fully initialized by request1 - Request r2 = client.newRequest("http://localhost:" + port + "/r2"); - r2.method(HttpMethod.GET).send(l); - - l.awaitCompletion(); - - assertThat(results, is(array(equalTo(HttpStatus.OK_200), equalTo(HttpStatus.OK_200)))); + + // Req1: should initialize servlet. + client.newRequest("http://localhost:" + port + "/r1").send(l); + + // Need to give 1st request a head start before request2. + Thread.sleep(DemoServlet.INIT_SLEEP / 4); + + // Req2: should see servlet fully initialized by request1. + client.newRequest("http://localhost:" + port + "/r2").send(l); + + assertTrue(resultsLatch.await(DemoServlet.INIT_SLEEP * 2, TimeUnit.MILLISECONDS)); + assertEquals(HttpStatus.OK_200, results[0]); + assertEquals(HttpStatus.OK_200, results[1]); } finally { - client.stop(); server.stop(); } }