Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue #5378 - add test to reproduce issue with WebSocketUpgradeFilter not started #5405

Merged
merged 3 commits into from Oct 8, 2020
Merged

Issue #5378 - add test to reproduce issue with WebSocketUpgradeFilter not started #5405

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2a4d672
Issue #5378 - improve testing for WebSocketUpgradeFilter
lachlan-roberts Oct 6, 2020
7003b3c
Issue #5378 - guard against concurrent requests lazily initializing t…
lachlan-roberts Oct 7, 2020
d2beb28
Merge remote-tracking branch 'origin/jetty-10.0.x' into jetty-10.0.x-…
lachlan-roberts Oct 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
93 changes: 89 additions & 4 deletions ...jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/JettyWebSocketFilterTest.java
View file
Open in desktop
Expand Up @@ -21,19 +21,24 @@
import java.net.URI;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.TimeUnit;
import javax.servlet.http.HttpServlet;

import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.ServerConnector;
import org.eclipse.jetty.servlet.FilterHolder;
import org.eclipse.jetty.servlet.ServletContextHandler;
import org.eclipse.jetty.servlet.ServletHolder;
import org.eclipse.jetty.util.component.AbstractLifeCycle;
import org.eclipse.jetty.websocket.api.Session;
import org.eclipse.jetty.websocket.client.WebSocketClient;
import org.eclipse.jetty.websocket.server.JettyWebSocketServerContainer;
import org.eclipse.jetty.websocket.server.config.JettyWebSocketServletContainerInitializer;
import org.eclipse.jetty.websocket.util.server.WebSocketUpgradeFilter;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.is;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
Expand All @@ -45,18 +50,29 @@ public class JettyWebSocketFilterTest
private WebSocketClient client;
private ServletContextHandler contextHandler;

@BeforeEach
public void start() throws Exception
public void start(JettyWebSocketServletContainerInitializer.Configurator configurator) throws Exception
{
start(configurator, null);
}

public void start(ServletHolder servletHolder) throws Exception
{
start(null, servletHolder);
}

public void start(JettyWebSocketServletContainerInitializer.Configurator configurator, ServletHolder servletHolder) throws Exception
{
server = new Server();
connector = new ServerConnector(server);
server.addConnector(connector);

contextHandler = new ServletContextHandler(ServletContextHandler.SESSIONS);
contextHandler.setContextPath("/");
if (servletHolder != null)
contextHandler.addServlet(servletHolder, "/");
server.setHandler(contextHandler);

JettyWebSocketServletContainerInitializer.configure(contextHandler, null);
JettyWebSocketServletContainerInitializer.configure(contextHandler, configurator);
gregw marked this conversation as resolved.
Show resolved Hide resolved
server.start();

client = new WebSocketClient();
Expand All @@ -70,9 +86,37 @@ public void stop() throws Exception
server.stop();
}

@Test
public void testWebSocketUpgradeFilter() throws Exception
{
start((context, container) -> container.addMapping("/", EchoSocket.class));

// After mapping is added we have an UpgradeFilter.
assertThat(contextHandler.getServletHandler().getFilters().length, is(1));
FilterHolder filterHolder = contextHandler.getServletHandler().getFilter("WebSocketUpgradeFilter");
assertNotNull(filterHolder);
assertThat(filterHolder.getState(), is(AbstractLifeCycle.STARTED));
assertThat(filterHolder.getFilter(), instanceOf(WebSocketUpgradeFilter.class));

// Test we can upgrade to websocket and send a message.
URI uri = URI.create("ws://localhost:" + connector.getLocalPort() + "/filterPath");
EventSocket socket = new EventSocket();
CompletableFuture<Session> connect = client.connect(socket, uri);
try (Session session = connect.get(5, TimeUnit.SECONDS))
{
session.getRemote().sendString("hello world");
}
assertTrue(socket.closeLatch.await(10, TimeUnit.SECONDS));

String msg = socket.textMessages.poll();
assertThat(msg, is("hello world"));
}

@Test
public void testLazyWebSocketUpgradeFilter() throws Exception
{
start(null, null);

// JettyWebSocketServerContainer has already been created.
JettyWebSocketServerContainer container = JettyWebSocketServerContainer.getContainer(contextHandler.getServletContext());
assertNotNull(container);
Expand All @@ -83,6 +127,47 @@ public void testLazyWebSocketUpgradeFilter() throws Exception
// After mapping is added we have an UpgradeFilter.
container.addMapping("/", EchoSocket.class);
assertThat(contextHandler.getServletHandler().getFilters().length, is(1));
FilterHolder filterHolder = contextHandler.getServletHandler().getFilter("WebSocketUpgradeFilter");
assertNotNull(filterHolder);
assertThat(filterHolder.getState(), is(AbstractLifeCycle.STARTED));
assertThat(filterHolder.getFilter(), instanceOf(WebSocketUpgradeFilter.class));

// Test we can upgrade to websocket and send a message.
URI uri = URI.create("ws://localhost:" + connector.getLocalPort() + "/filterPath");
EventSocket socket = new EventSocket();
CompletableFuture<Session> connect = client.connect(socket, uri);
try (Session session = connect.get(5, TimeUnit.SECONDS))
{
session.getRemote().sendString("hello world");
}
assertTrue(socket.closeLatch.await(10, TimeUnit.SECONDS));

String msg = socket.textMessages.poll();
assertThat(msg, is("hello world"));
}

@Test
public void testWebSocketUpgradeFilterWhileStarting() throws Exception
{
start(new ServletHolder(new HttpServlet()
{
@Override
public void init()
{
JettyWebSocketServerContainer container = JettyWebSocketServerContainer.getContainer(getServletContext());
if (container == null)
throw new IllegalArgumentException("Missing JettyWebSocketServerContainer");

container.addMapping("/", EchoSocket.class);
}
}));

// After mapping is added we have an UpgradeFilter.
assertThat(contextHandler.getServletHandler().getFilters().length, is(1));
FilterHolder filterHolder = contextHandler.getServletHandler().getFilter("WebSocketUpgradeFilter");
assertNotNull(filterHolder);
assertThat(filterHolder.getState(), is(AbstractLifeCycle.STARTED));
assertThat(filterHolder.getFilter(), instanceOf(WebSocketUpgradeFilter.class));

// Test we can upgrade to websocket and send a message.
URI uri = URI.create("ws://localhost:" + connector.getLocalPort() + "/filterPath");
Expand Down
43 changes: 26 additions & 17 deletions ...-server/src/main/java/org/eclipse/jetty/websocket/util/server/WebSocketUpgradeFilter.java
View file
Open in desktop
Expand Up @@ -21,6 +21,7 @@
import java.io.IOException;
import java.time.Duration;
import java.util.EnumSet;
import java.util.Objects;
import javax.servlet.DispatcherType;
import javax.servlet.Filter;
import javax.servlet.FilterChain;
Expand All @@ -37,6 +38,7 @@
import org.eclipse.jetty.servlet.ServletHandler;
import org.eclipse.jetty.util.annotation.ManagedObject;
import org.eclipse.jetty.util.component.Dumpable;
import org.eclipse.jetty.util.thread.AutoLock;
import org.eclipse.jetty.websocket.core.Configuration;
import org.eclipse.jetty.websocket.core.server.WebSocketServerComponents;
import org.eclipse.jetty.websocket.util.server.internal.WebSocketMapping;
Expand Down Expand Up @@ -74,10 +76,12 @@
public class WebSocketUpgradeFilter implements Filter, Dumpable
{
private static final Logger LOG = LoggerFactory.getLogger(WebSocketUpgradeFilter.class);
private static final AutoLock LOCK = new AutoLock();

private static FilterHolder getFilter(ServletContext servletContext)
{
ServletHandler servletHandler = ContextHandler.getContextHandler(servletContext).getChildHandlerByClass(ServletHandler.class);
ContextHandler contextHandler = Objects.requireNonNull(ContextHandler.getContextHandler(servletContext));
ServletHandler servletHandler = contextHandler.getChildHandlerByClass(ServletHandler.class);

for (FilterHolder holder : servletHandler.getFilters())
{
Expand Down Expand Up @@ -105,22 +109,27 @@ private static FilterHolder getFilter(ServletContext servletContext)
*/
public static FilterHolder ensureFilter(ServletContext servletContext)
{
FilterHolder existingFilter = WebSocketUpgradeFilter.getFilter(servletContext);
if (existingFilter != null)
return existingFilter;

final String name = "WebSocketUpgradeFilter";
final String pathSpec = "/*";
FilterHolder holder = new FilterHolder(new WebSocketUpgradeFilter());
holder.setName(name);
holder.setInitParameter(MAPPING_ATTRIBUTE_INIT_PARAM, WebSocketMapping.DEFAULT_KEY);

holder.setAsyncSupported(true);
ServletHandler servletHandler = ContextHandler.getContextHandler(servletContext).getChildHandlerByClass(ServletHandler.class);
servletHandler.addFilterWithMapping(holder, pathSpec, EnumSet.of(DispatcherType.REQUEST));
if (LOG.isDebugEnabled())
LOG.debug("Adding {} mapped to {} in {}", holder, pathSpec, servletContext);
return holder;
// Lock in case two concurrent requests are initializing the filter lazily.
try (AutoLock l = LOCK.lock())
gregw marked this conversation as resolved.
Show resolved Hide resolved
{
FilterHolder existingFilter = WebSocketUpgradeFilter.getFilter(servletContext);
if (existingFilter != null)
return existingFilter;

final String name = "WebSocketUpgradeFilter";
final String pathSpec = "/*";
FilterHolder holder = new FilterHolder(new WebSocketUpgradeFilter());
holder.setName(name);
holder.setInitParameter(MAPPING_ATTRIBUTE_INIT_PARAM, WebSocketMapping.DEFAULT_KEY);

holder.setAsyncSupported(true);
ContextHandler contextHandler = Objects.requireNonNull(ContextHandler.getContextHandler(servletContext));
ServletHandler servletHandler = contextHandler.getChildHandlerByClass(ServletHandler.class);
servletHandler.addFilterWithMapping(holder, pathSpec, EnumSet.of(DispatcherType.REQUEST));
if (LOG.isDebugEnabled())
LOG.debug("Adding {} mapped to {} in {}", holder, pathSpec, servletContext);
return holder;
}
}

public static final String MAPPING_ATTRIBUTE_INIT_PARAM = "org.eclipse.jetty.websocket.util.server.internal.WebSocketMapping.key";
Expand Down