You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2019/06/21 19:44:59 UTC
[tomcat] branch 8.5.x updated: Fix
https://bz.apache.org/bugzilla/show_bug.cgi?id=63521
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 8.5.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/8.5.x by this push:
new b61d651 Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=63521
b61d651 is described below
commit b61d651de3698683900a0cc6f19f27ae58f96086
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Fri Jun 21 20:36:37 2019 +0100
Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=63521
As required by the WebSocket specification, if a POJO that is deployed
as a result of the SCI scan for annotated POJOs is subsequently deployed
via the programmatic API ignore the programmatic deployment.
---
java/org/apache/tomcat/websocket/server/WsSci.java | 2 +-
.../tomcat/websocket/server/WsServerContainer.java | 125 +++++++++------
.../websocket/server/TestWsServerContainer.java | 171 ++++++++++++++++++---
webapps/docs/changelog.xml | 10 ++
4 files changed, 239 insertions(+), 69 deletions(-)
diff --git a/java/org/apache/tomcat/websocket/server/WsSci.java b/java/org/apache/tomcat/websocket/server/WsSci.java
index 73dabd7..88d2462 100644
--- a/java/org/apache/tomcat/websocket/server/WsSci.java
+++ b/java/org/apache/tomcat/websocket/server/WsSci.java
@@ -117,7 +117,7 @@ public class WsSci implements ServletContainerInitializer {
}
// Deploy POJOs
for (Class<?> clazz : filteredPojoEndpoints) {
- sc.addEndpoint(clazz);
+ sc.addEndpoint(clazz, true);
}
} catch (DeploymentException e) {
throw new ServletException(e);
diff --git a/java/org/apache/tomcat/websocket/server/WsServerContainer.java b/java/org/apache/tomcat/websocket/server/WsServerContainer.java
index 63045d4..f2238fc 100644
--- a/java/org/apache/tomcat/websocket/server/WsServerContainer.java
+++ b/java/org/apache/tomcat/websocket/server/WsServerContainer.java
@@ -19,14 +19,12 @@ package org.apache.tomcat.websocket.server;
import java.io.IOException;
import java.util.Arrays;
import java.util.Collections;
-import java.util.Comparator;
import java.util.EnumSet;
import java.util.Map;
import java.util.Set;
-import java.util.SortedSet;
-import java.util.TreeSet;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.ConcurrentSkipListMap;
import javax.servlet.DispatcherType;
import javax.servlet.FilterRegistration;
@@ -72,9 +70,8 @@ public class WsServerContainer extends WsWebSocketContainer
private final WsWriteTimeout wsWriteTimeout = new WsWriteTimeout();
private final ServletContext servletContext;
- private final Map<String,ServerEndpointConfig> configExactMatchMap =
- new ConcurrentHashMap<>();
- private final ConcurrentMap<Integer,SortedSet<TemplatePathMatch>> configTemplateMatchMap =
+ private final Map<String,ExactPathMatch> configExactMatchMap = new ConcurrentHashMap<>();
+ private final ConcurrentMap<Integer,ConcurrentSkipListMap<String,TemplatePathMatch>> configTemplateMatchMap =
new ConcurrentHashMap<>();
private volatile boolean enforceNoAddAfterHandshake =
org.apache.tomcat.websocket.Constants.STRICT_SPEC_COMPLIANCE;
@@ -130,6 +127,11 @@ public class WsServerContainer extends WsWebSocketContainer
*/
@Override
public void addEndpoint(ServerEndpointConfig sec) throws DeploymentException {
+ addEndpoint(sec, false);
+ }
+
+
+ void addEndpoint(ServerEndpointConfig sec, boolean fromAnnotatedPojo) throws DeploymentException {
if (enforceNoAddAfterHandshake && !addAllowed) {
throw new DeploymentException(
@@ -161,32 +163,50 @@ public class WsServerContainer extends WsWebSocketContainer
UriTemplate uriTemplate = new UriTemplate(path);
if (uriTemplate.hasParameters()) {
Integer key = Integer.valueOf(uriTemplate.getSegmentCount());
- SortedSet<TemplatePathMatch> templateMatches =
+ ConcurrentSkipListMap<String,TemplatePathMatch> templateMatches =
configTemplateMatchMap.get(key);
if (templateMatches == null) {
// Ensure that if concurrent threads execute this block they
- // both end up using the same TreeSet instance
- templateMatches = new TreeSet<>(
- TemplatePathMatchComparator.getInstance());
+ // all end up using the same ConcurrentSkipListMap instance
+ templateMatches = new ConcurrentSkipListMap<>();
configTemplateMatchMap.putIfAbsent(key, templateMatches);
templateMatches = configTemplateMatchMap.get(key);
}
- if (!templateMatches.add(new TemplatePathMatch(sec, uriTemplate))) {
- // Duplicate uriTemplate;
- throw new DeploymentException(
- sm.getString("serverContainer.duplicatePaths", path,
- sec.getEndpointClass(),
- sec.getEndpointClass()));
+ TemplatePathMatch newMatch = new TemplatePathMatch(sec, uriTemplate, fromAnnotatedPojo);
+ TemplatePathMatch oldMatch = templateMatches.putIfAbsent(uriTemplate.getNormalizedPath(), newMatch);
+ if (oldMatch != null) {
+ // Note: This depends on Endpoint instances being added
+ // before POJOs in WsSci#onStartup()
+ if (oldMatch.isFromAnnotatedPojo() && !newMatch.isFromAnnotatedPojo() &&
+ oldMatch.getConfig().getEndpointClass() == newMatch.getConfig().getEndpointClass()) {
+ // The WebSocket spec says to ignore the new match in this case
+ templateMatches.put(path, oldMatch);
+ } else {
+ // Duplicate uriTemplate;
+ throw new DeploymentException(
+ sm.getString("serverContainer.duplicatePaths", path,
+ sec.getEndpointClass(),
+ sec.getEndpointClass()));
+ }
}
} else {
// Exact match
- ServerEndpointConfig old = configExactMatchMap.put(path, sec);
- if (old != null) {
- // Duplicate path mappings
- throw new DeploymentException(
- sm.getString("serverContainer.duplicatePaths", path,
- old.getEndpointClass(),
- sec.getEndpointClass()));
+ ExactPathMatch newMatch = new ExactPathMatch(sec, fromAnnotatedPojo);
+ ExactPathMatch oldMatch = configExactMatchMap.put(path, newMatch);
+ if (oldMatch != null) {
+ // Note: This depends on Endpoint instances being added
+ // before POJOs in WsSci#onStartup()
+ if (oldMatch.isFromAnnotatedPojo() && !newMatch.isFromAnnotatedPojo() &&
+ oldMatch.getConfig().getEndpointClass() == newMatch.getConfig().getEndpointClass()) {
+ // The WebSocket spec says to ignore the new match in this case
+ configExactMatchMap.put(path, oldMatch);
+ } else {
+ // Duplicate path mappings
+ throw new DeploymentException(
+ sm.getString("serverContainer.duplicatePaths", path,
+ oldMatch.getConfig().getEndpointClass(),
+ sec.getEndpointClass()));
+ }
}
}
@@ -207,6 +227,11 @@ public class WsServerContainer extends WsWebSocketContainer
*/
@Override
public void addEndpoint(Class<?> pojo) throws DeploymentException {
+ addEndpoint(pojo, false);
+ }
+
+
+ void addEndpoint(Class<?> pojo, boolean fromAnnotatedPojo) throws DeploymentException {
if (deploymentFailed) {
throw new DeploymentException(sm.getString("serverContainer.failedDeployment",
@@ -252,7 +277,7 @@ public class WsServerContainer extends WsWebSocketContainer
throw de;
}
- addEndpoint(sec);
+ addEndpoint(sec, fromAnnotatedPojo);
}
@@ -306,9 +331,9 @@ public class WsServerContainer extends WsWebSocketContainer
}
// Check an exact match. Simple case as there are no templates.
- ServerEndpointConfig sec = configExactMatchMap.get(path);
- if (sec != null) {
- return new WsMappingResult(sec, Collections.<String, String>emptyMap());
+ ExactPathMatch match = configExactMatchMap.get(path);
+ if (match != null) {
+ return new WsMappingResult(match.getConfig(), Collections.<String, String>emptyMap());
}
// No exact match. Need to look for template matches.
@@ -322,8 +347,7 @@ public class WsServerContainer extends WsWebSocketContainer
// Number of segments has to match
Integer key = Integer.valueOf(pathUriTemplate.getSegmentCount());
- SortedSet<TemplatePathMatch> templateMatches =
- configTemplateMatchMap.get(key);
+ ConcurrentSkipListMap<String,TemplatePathMatch> templateMatches = configTemplateMatchMap.get(key);
if (templateMatches == null) {
// No templates with an equal number of segments so there will be
@@ -333,8 +357,9 @@ public class WsServerContainer extends WsWebSocketContainer
// List is in alphabetical order of normalised templates.
// Correct match is the first one that matches.
+ ServerEndpointConfig sec = null;
Map<String,String> pathParams = null;
- for (TemplatePathMatch templateMatch : templateMatches) {
+ for (TemplatePathMatch templateMatch : templateMatches.values()) {
pathParams = templateMatch.getUriTemplate().match(pathUriTemplate);
if (pathParams != null) {
sec = templateMatch.getConfig();
@@ -461,11 +486,13 @@ public class WsServerContainer extends WsWebSocketContainer
private static class TemplatePathMatch {
private final ServerEndpointConfig config;
private final UriTemplate uriTemplate;
+ private final boolean fromAnnotatedPojo;
- public TemplatePathMatch(ServerEndpointConfig config,
- UriTemplate uriTemplate) {
+ public TemplatePathMatch(ServerEndpointConfig config, UriTemplate uriTemplate,
+ boolean fromAnnotatedPojo) {
this.config = config;
this.uriTemplate = uriTemplate;
+ this.fromAnnotatedPojo = fromAnnotatedPojo;
}
@@ -477,31 +504,31 @@ public class WsServerContainer extends WsWebSocketContainer
public UriTemplate getUriTemplate() {
return uriTemplate;
}
- }
- /**
- * This Comparator implementation is thread-safe so only create a single
- * instance.
- */
- private static class TemplatePathMatchComparator
- implements Comparator<TemplatePathMatch> {
+ public boolean isFromAnnotatedPojo() {
+ return fromAnnotatedPojo;
+ }
+ }
- private static final TemplatePathMatchComparator INSTANCE =
- new TemplatePathMatchComparator();
- public static TemplatePathMatchComparator getInstance() {
- return INSTANCE;
+ private static class ExactPathMatch {
+ private final ServerEndpointConfig config;
+ private final boolean fromAnnotatedPojo;
+
+ public ExactPathMatch(ServerEndpointConfig config, boolean fromAnnotatedPojo) {
+ this.config = config;
+ this.fromAnnotatedPojo = fromAnnotatedPojo;
}
- private TemplatePathMatchComparator() {
- // Hide default constructor
+
+ public ServerEndpointConfig getConfig() {
+ return config;
}
- @Override
- public int compare(TemplatePathMatch tpm1, TemplatePathMatch tpm2) {
- return tpm1.getUriTemplate().getNormalizedPath().compareTo(
- tpm2.getUriTemplate().getNormalizedPath());
+
+ public boolean isFromAnnotatedPojo() {
+ return fromAnnotatedPojo;
}
}
}
diff --git a/test/org/apache/tomcat/websocket/server/TestWsServerContainer.java b/test/org/apache/tomcat/websocket/server/TestWsServerContainer.java
index 08acf45..8beb326 100644
--- a/test/org/apache/tomcat/websocket/server/TestWsServerContainer.java
+++ b/test/org/apache/tomcat/websocket/server/TestWsServerContainer.java
@@ -22,8 +22,10 @@ import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
import javax.websocket.ContainerProvider;
+import javax.websocket.DeploymentException;
import javax.websocket.Session;
import javax.websocket.WebSocketContainer;
+import javax.websocket.server.ServerEndpoint;
import javax.websocket.server.ServerEndpointConfig;
import org.junit.Assert;
@@ -106,8 +108,7 @@ public class TestWsServerContainer extends WebSocketBaseTest {
@Test
public void testSpecExample3() throws Exception {
- WsServerContainer sc =
- new WsServerContainer(new TesterServletContext());
+ WsServerContainer sc = new WsServerContainer(new TesterServletContext());
ServerEndpointConfig configA = ServerEndpointConfig.Builder.create(
Object.class, "/a/{var}/c").build();
@@ -128,8 +129,7 @@ public class TestWsServerContainer extends WebSocketBaseTest {
@Test
public void testSpecExample4() throws Exception {
- WsServerContainer sc =
- new WsServerContainer(new TesterServletContext());
+ WsServerContainer sc = new WsServerContainer(new TesterServletContext());
ServerEndpointConfig configA = ServerEndpointConfig.Builder.create(
Object.class, "/{var1}/d").build();
@@ -143,10 +143,9 @@ public class TestWsServerContainer extends WebSocketBaseTest {
}
- @Test(expected = javax.websocket.DeploymentException.class)
- public void testDuplicatePaths_01() throws Exception {
- WsServerContainer sc =
- new WsServerContainer(new TesterServletContext());
+ @Test(expected = DeploymentException.class)
+ public void testDuplicatePaths01() throws Exception {
+ WsServerContainer sc = new WsServerContainer(new TesterServletContext());
ServerEndpointConfig configA = ServerEndpointConfig.Builder.create(
Object.class, "/a/b/c").build();
@@ -158,10 +157,9 @@ public class TestWsServerContainer extends WebSocketBaseTest {
}
- @Test(expected = javax.websocket.DeploymentException.class)
- public void testDuplicatePaths_02() throws Exception {
- WsServerContainer sc =
- new WsServerContainer(new TesterServletContext());
+ @Test(expected = DeploymentException.class)
+ public void testDuplicatePaths02() throws Exception {
+ WsServerContainer sc = new WsServerContainer(new TesterServletContext());
ServerEndpointConfig configA = ServerEndpointConfig.Builder.create(
Object.class, "/a/b/{var}").build();
@@ -173,10 +171,9 @@ public class TestWsServerContainer extends WebSocketBaseTest {
}
- @Test(expected = javax.websocket.DeploymentException.class)
- public void testDuplicatePaths_03() throws Exception {
- WsServerContainer sc =
- new WsServerContainer(new TesterServletContext());
+ @Test(expected = DeploymentException.class)
+ public void testDuplicatePaths03() throws Exception {
+ WsServerContainer sc = new WsServerContainer(new TesterServletContext());
ServerEndpointConfig configA = ServerEndpointConfig.Builder.create(
Object.class, "/a/b/{var1}").build();
@@ -189,9 +186,8 @@ public class TestWsServerContainer extends WebSocketBaseTest {
@Test
- public void testDuplicatePaths_04() throws Exception {
- WsServerContainer sc =
- new WsServerContainer(new TesterServletContext());
+ public void testDuplicatePaths04() throws Exception {
+ WsServerContainer sc = new WsServerContainer(new TesterServletContext());
ServerEndpointConfig configA = ServerEndpointConfig.Builder.create(
Object.class, "/a/{var1}/{var2}").build();
@@ -204,4 +200,141 @@ public class TestWsServerContainer extends WebSocketBaseTest {
Assert.assertEquals(configA, sc.findMapping("/a/x/y").getConfig());
Assert.assertEquals(configB, sc.findMapping("/a/b/y").getConfig());
}
+
+
+ /*
+ * Simulates a class that gets picked up for extending Endpoint and for
+ * being annotated.
+ */
+ @Test(expected = DeploymentException.class)
+ public void testDuplicatePaths11() throws Exception {
+ WsServerContainer sc = new WsServerContainer(new TesterServletContext());
+
+ ServerEndpointConfig configA = ServerEndpointConfig.Builder.create(
+ Pojo.class, "/foo").build();
+
+ sc.addEndpoint(configA, false);
+ sc.addEndpoint(Pojo.class, true);
+ }
+
+
+ /*
+ * POJO auto deployment followed by programmatic duplicate. Keep POJO.
+ */
+ @Test
+ public void testDuplicatePaths12() throws Exception {
+ WsServerContainer sc = new WsServerContainer(new TesterServletContext());
+
+ ServerEndpointConfig configA = ServerEndpointConfig.Builder.create(
+ Pojo.class, "/foo").build();
+
+ sc.addEndpoint(Pojo.class, true);
+ sc.addEndpoint(configA);
+
+ Assert.assertNotEquals(configA, sc.findMapping("/foo").getConfig());
+ }
+
+
+ /*
+ * POJO programmatic followed by programmatic duplicate.
+ */
+ @Test(expected = DeploymentException.class)
+ public void testDuplicatePaths13() throws Exception {
+ WsServerContainer sc = new WsServerContainer(new TesterServletContext());
+
+ ServerEndpointConfig configA = ServerEndpointConfig.Builder.create(
+ Pojo.class, "/foo").build();
+
+ sc.addEndpoint(Pojo.class);
+ sc.addEndpoint(configA);
+ }
+
+
+ /*
+ * POJO auto deployment followed by programmatic on same path.
+ */
+ @Test(expected = DeploymentException.class)
+ public void testDuplicatePaths14() throws Exception {
+ WsServerContainer sc = new WsServerContainer(new TesterServletContext());
+
+ ServerEndpointConfig configA = ServerEndpointConfig.Builder.create(
+ Object.class, "/foo").build();
+
+ sc.addEndpoint(Pojo.class, true);
+ sc.addEndpoint(configA);
+ }
+
+
+ /*
+ * Simulates a class that gets picked up for extending Endpoint and for
+ * being annotated.
+ */
+ @Test(expected = DeploymentException.class)
+ public void testDuplicatePaths21() throws Exception {
+ WsServerContainer sc = new WsServerContainer(new TesterServletContext());
+
+ ServerEndpointConfig configA = ServerEndpointConfig.Builder.create(
+ PojoTemplate.class, "/foo/{a}").build();
+
+ sc.addEndpoint(configA, false);
+ sc.addEndpoint(PojoTemplate.class, true);
+ }
+
+
+ /*
+ * POJO auto deployment followed by programmatic duplicate. Keep POJO.
+ */
+ @Test
+ public void testDuplicatePaths22() throws Exception {
+ WsServerContainer sc = new WsServerContainer(new TesterServletContext());
+
+ ServerEndpointConfig configA = ServerEndpointConfig.Builder.create(
+ PojoTemplate.class, "/foo/{a}").build();
+
+ sc.addEndpoint(PojoTemplate.class, true);
+ sc.addEndpoint(configA);
+
+ Assert.assertNotEquals(configA, sc.findMapping("/foo/{a}").getConfig());
+ }
+
+
+ /*
+ * POJO programmatic followed by programmatic duplicate.
+ */
+ @Test(expected = DeploymentException.class)
+ public void testDuplicatePaths23() throws Exception {
+ WsServerContainer sc = new WsServerContainer(new TesterServletContext());
+
+ ServerEndpointConfig configA = ServerEndpointConfig.Builder.create(
+ PojoTemplate.class, "/foo/{a}").build();
+
+ sc.addEndpoint(PojoTemplate.class);
+ sc.addEndpoint(configA);
+ }
+
+
+ /*
+ * POJO auto deployment followed by programmatic on same path.
+ */
+ @Test(expected = DeploymentException.class)
+ public void testDuplicatePaths24() throws Exception {
+ WsServerContainer sc = new WsServerContainer(new TesterServletContext());
+
+ ServerEndpointConfig configA = ServerEndpointConfig.Builder.create(
+ Object.class, "/foo/{a}").build();
+
+ sc.addEndpoint(PojoTemplate.class, true);
+ sc.addEndpoint(configA);
+ }
+
+
+ @ServerEndpoint("/foo")
+ public static class Pojo {
+ }
+
+
+ @ServerEndpoint("/foo/{a}")
+ public static class PojoTemplate {
+ }
+
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index dcabca3..c86abb4 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -65,6 +65,16 @@
</add>
</changelog>
</subsection>
+ <subsection name="WebSocket">
+ <changelog>
+ <fix>
+ <bug>63521</bug>: As required by the WebSocket specification, if a POJO
+ that is deployed as a result of the SCI scan for annotated POJOs is
+ subsequently deployed via the programmatic API ignore the programmatic
+ deployment. (markt)
+ </fix>
+ </changelog>
+ </subsection>
</section>
<section name="Tomcat 8.5.42 (markt)" rtext="2019-06-07">
<subsection name="Catalina">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org