markt-asf commented on a change in pull request #346:
URL: https://github.com/apache/tomcat/pull/346#discussion_r481028956
##########
File path: test/org/apache/tomcat/websocket/TestWsWebSocketContainer.java
##########
@@ -779,6 +779,104 @@ public void testSessionExpirySession() throws Exception {
Assert.assertEquals(0, getOpenCount(setA));
}
+ @Test
+ public void testSessionExpiryOnUserPropertyReadIdleTimeout() throws
Exception {
+ final String readIdleTimeoutParameter =
org.apache.tomcat.websocket.Constants.WS_READ_IDLE_TIMEOUT;
+
+ Tomcat tomcat = getTomcatInstance();
+ // No file system docBase required
+ Context ctx = tomcat.addContext("", null);
+ ctx.addApplicationListener(TesterEchoServer.Config.class.getName());
+ Tomcat.addServlet(ctx, "default", new DefaultServlet());
+ ctx.addServletMappingDecoded("/", "default");
+
+ tomcat.start();
+
+ // Need access to implementation methods for configuring unit tests
+ WsWebSocketContainer wsContainer = (WsWebSocketContainer)
+ ContainerProvider.getWebSocketContainer();
+
+ wsContainer.setDefaultMaxSessionIdleTimeout(5000);
+ wsContainer.setProcessPeriod(1);
+
+ EndpointA endpointA = new EndpointA();
+ Session s1a = connectToEchoServer(wsContainer, endpointA,
+ TesterEchoServer.Config.PATH_BASIC);
+ s1a.setMaxIdleTimeout(90000);
+
+ s1a.getUserProperties().put(readIdleTimeoutParameter, (long)5000);
Review comment:
Using Long here avoids an IDE warning. (and the same in the second new
test below).
##########
File path: java/org/apache/tomcat/websocket/WsSession.java
##########
@@ -816,16 +823,62 @@ protected void checkExpiration() {
return;
}
- if (System.currentTimeMillis() - lastActive > timeout) {
+ long currentTime = System.currentTimeMillis();
+
+ if (currentTime - lastActive > timeout) {
String msg = sm.getString("wsSession.timeout", getId());
if (log.isDebugEnabled()) {
log.debug(msg);
}
doClose(new CloseReason(CloseCodes.GOING_AWAY, msg),
new CloseReason(CloseCodes.CLOSED_ABNORMALLY, msg));
+ } else {
+ boolean closeConnection = false;
+ Long idleReadTimeout = getIdleReadTimeout();
+
+ closeConnection = idleReadTimeout != null && (currentTime -
lastReadTime > idleReadTimeout);
+
+ if (closeConnection) {
+ String msg = sm.getString("wsSession.readIdleTimeout",
getId());
+ if (log.isDebugEnabled()) {
+ log.debug(msg);
+ }
+ doClose(new CloseReason(CloseCodes.GOING_AWAY, msg),
+ new CloseReason(CloseCodes.CLOSED_ABNORMALLY, msg));
+ }
+
+ Long idleWriteTimeout = getIdleWriteTimeout();
+
+ closeConnection = idleWriteTimeout != null && (currentTime -
lastWriteTime > idleWriteTimeout);
+
+ if (closeConnection) {
+ String msg = sm.getString("wsSession.writeIdleTimeout",
getId());
+ if (log.isDebugEnabled()) {
+ log.debug(msg);
+ }
+ doClose(new CloseReason(CloseCodes.GOING_AWAY, msg),
+ new CloseReason(CloseCodes.CLOSED_ABNORMALLY, msg));
+ }
}
}
+ private Long getIdleReadTimeout() {
+ Object readTimeout =
this.getUserProperties().get(Constants.WS_READ_IDLE_TIMEOUT);
+ Long userReadTimeout = null;
+ if (readTimeout instanceof Long) {
+ userReadTimeout = (Long) readTimeout;
+ }
+ return userReadTimeout;
+ }
+
+ private Long getIdleWriteTimeout() {
+ Object writeTimeout =
this.getUserProperties().get(Constants.WS_WRITE_IDLE_TIMEOUT);
+ Long userWriteTimeout = null;
Review comment:
No need for the additional reference here
##########
File path: test/org/apache/tomcat/websocket/TestWsWebSocketContainer.java
##########
@@ -779,6 +779,104 @@ public void testSessionExpirySession() throws Exception {
Assert.assertEquals(0, getOpenCount(setA));
}
+ @Test
+ public void testSessionExpiryOnUserPropertyReadIdleTimeout() throws
Exception {
+ final String readIdleTimeoutParameter =
org.apache.tomcat.websocket.Constants.WS_READ_IDLE_TIMEOUT;
+
+ Tomcat tomcat = getTomcatInstance();
+ // No file system docBase required
+ Context ctx = tomcat.addContext("", null);
+ ctx.addApplicationListener(TesterEchoServer.Config.class.getName());
+ Tomcat.addServlet(ctx, "default", new DefaultServlet());
+ ctx.addServletMappingDecoded("/", "default");
+
+ tomcat.start();
+
+ // Need access to implementation methods for configuring unit tests
+ WsWebSocketContainer wsContainer = (WsWebSocketContainer)
+ ContainerProvider.getWebSocketContainer();
+
+ wsContainer.setDefaultMaxSessionIdleTimeout(5000);
+ wsContainer.setProcessPeriod(1);
+
+ EndpointA endpointA = new EndpointA();
+ Session s1a = connectToEchoServer(wsContainer, endpointA,
+ TesterEchoServer.Config.PATH_BASIC);
+ s1a.setMaxIdleTimeout(90000);
+
+ s1a.getUserProperties().put(readIdleTimeoutParameter, (long)5000);
+
+ // even though the maxIdleTimeout is 1.5 minutes but, the
readIdleTimeout is 5s.
+ // Thus, session should get closed after 5 seconds as nothing is read
on it and user has set the
+ // max idle read timeout parameter.
+ Set<Session> setA = s1a.getOpenSessions();
+ int expected = 1;
+ while (expected > 0) {
+ Assert.assertEquals(expected, getOpenCount(setA));
+ int count = 0;
+ while (getOpenCount(setA) == expected && count < 50) {
+ count ++;
+ Thread.sleep(100);
+ }
+ expected--;
+ }
+ // to make sure the background thread to check expiration runs
+ Thread.sleep(2000);
+ Assert.assertEquals(0, getOpenCount(setA));
+ }
+
+ @Test
+ public void testSessionExpiryOnUserPropertyWriteIdleTimeout() throws
Exception {
+ final String writeIdleTimeoutParameter =
org.apache.tomcat.websocket.Constants.WS_WRITE_IDLE_TIMEOUT;
+
+ Tomcat tomcat = getTomcatInstance();
+ // No file system docBase required
+ Context ctx = tomcat.addContext("", null);
+ ctx.addApplicationListener(TesterEchoServer.Config.class.getName());
+ Tomcat.addServlet(ctx, "default", new DefaultServlet());
+ ctx.addServletMappingDecoded("/", "default");
+
+ tomcat.start();
+
+ // Need access to implementation methods for configuring unit tests
+ WsWebSocketContainer wsContainer = (WsWebSocketContainer)
+ ContainerProvider.getWebSocketContainer();
+
+ wsContainer.setDefaultMaxSessionIdleTimeout(5000);
Review comment:
This is not consistent with the comment that the session idle timeout is
1.5 minutes. And the same for the first test above.
##########
File path: java/org/apache/tomcat/websocket/Constants.java
##########
@@ -114,6 +114,14 @@
// Milliseconds so this is 20 seconds
public static final long DEFAULT_BLOCKING_SEND_TIMEOUT = 20 * 1000;
+ // Configuration for idle read timeout on websocket channel
+ public static final String WS_READ_IDLE_TIMEOUT =
+ "org.apache.tomcat.websocket.WS_READ_IDLE_TIMEOUT";
+
+ // Configuration for idle write timeout on websocket channel
+ public static final String WS_WRITE_IDLE_TIMEOUT =
+ "org.apache.tomcat.websocket.WS_WRITE_IDLE_TIMEOUT";
+
Review comment:
The current code isn't consistent but removing `WS_` from the start of
these properties and adding `_MS` to the end is more in line with the current
code.
##########
File path: test/org/apache/tomcat/websocket/TestWsWebSocketContainer.java
##########
@@ -779,6 +779,104 @@ public void testSessionExpirySession() throws Exception {
Assert.assertEquals(0, getOpenCount(setA));
}
+ @Test
+ public void testSessionExpiryOnUserPropertyReadIdleTimeout() throws
Exception {
+ final String readIdleTimeoutParameter =
org.apache.tomcat.websocket.Constants.WS_READ_IDLE_TIMEOUT;
Review comment:
I opted to use the Constants class (as it is in the same package) and
use the fully qualified name with the Constants from the server package.
##########
File path: test/org/apache/tomcat/websocket/TestWsWebSocketContainer.java
##########
@@ -779,6 +779,104 @@ public void testSessionExpirySession() throws Exception {
Assert.assertEquals(0, getOpenCount(setA));
}
+ @Test
+ public void testSessionExpiryOnUserPropertyReadIdleTimeout() throws
Exception {
+ final String readIdleTimeoutParameter =
org.apache.tomcat.websocket.Constants.WS_READ_IDLE_TIMEOUT;
+
+ Tomcat tomcat = getTomcatInstance();
+ // No file system docBase required
+ Context ctx = tomcat.addContext("", null);
+ ctx.addApplicationListener(TesterEchoServer.Config.class.getName());
+ Tomcat.addServlet(ctx, "default", new DefaultServlet());
+ ctx.addServletMappingDecoded("/", "default");
+
+ tomcat.start();
+
+ // Need access to implementation methods for configuring unit tests
+ WsWebSocketContainer wsContainer = (WsWebSocketContainer)
+ ContainerProvider.getWebSocketContainer();
+
+ wsContainer.setDefaultMaxSessionIdleTimeout(5000);
+ wsContainer.setProcessPeriod(1);
+
+ EndpointA endpointA = new EndpointA();
+ Session s1a = connectToEchoServer(wsContainer, endpointA,
+ TesterEchoServer.Config.PATH_BASIC);
+ s1a.setMaxIdleTimeout(90000);
+
+ s1a.getUserProperties().put(readIdleTimeoutParameter, (long)5000);
+
+ // even though the maxIdleTimeout is 1.5 minutes but, the
readIdleTimeout is 5s.
+ // Thus, session should get closed after 5 seconds as nothing is read
on it and user has set the
+ // max idle read timeout parameter.
+ Set<Session> setA = s1a.getOpenSessions();
+ int expected = 1;
+ while (expected > 0) {
+ Assert.assertEquals(expected, getOpenCount(setA));
+ int count = 0;
+ while (getOpenCount(setA) == expected && count < 50) {
+ count ++;
+ Thread.sleep(100);
+ }
+ expected--;
+ }
+ // to make sure the background thread to check expiration runs
+ Thread.sleep(2000);
+ Assert.assertEquals(0, getOpenCount(setA));
+ }
+
+ @Test
+ public void testSessionExpiryOnUserPropertyWriteIdleTimeout() throws
Exception {
+ final String writeIdleTimeoutParameter =
org.apache.tomcat.websocket.Constants.WS_WRITE_IDLE_TIMEOUT;
+
+ Tomcat tomcat = getTomcatInstance();
+ // No file system docBase required
+ Context ctx = tomcat.addContext("", null);
+ ctx.addApplicationListener(TesterEchoServer.Config.class.getName());
+ Tomcat.addServlet(ctx, "default", new DefaultServlet());
+ ctx.addServletMappingDecoded("/", "default");
+
+ tomcat.start();
+
+ // Need access to implementation methods for configuring unit tests
+ WsWebSocketContainer wsContainer = (WsWebSocketContainer)
+ ContainerProvider.getWebSocketContainer();
+
+ wsContainer.setDefaultMaxSessionIdleTimeout(5000);
+ wsContainer.setProcessPeriod(1);
+
+ EndpointA endpointA = new EndpointA();
+ Session s1a = connectToEchoServer(wsContainer, endpointA,
+ TesterEchoServer.Config.PATH_BASIC);
+ s1a.setMaxIdleTimeout(90000);
+
+ s1a.getUserProperties().put(writeIdleTimeoutParameter, (long)5000);
+
+ // write on session for 5 seconds to make sure the session is not idle
on write
+ int timesWritten = 0;
+ while(5 > timesWritten++) {
+ s1a.getBasicRemote().sendText(MESSAGE_STRING_1);
+ Thread.sleep(1000);
+ }
+
+ // even though the maxIdleTimeout is 1.5 minutes but, the
writeIdleTimeout is 5s.
+ // Thus, session should get closed after 5 seconds as nothing is being
written on it and user has set the
+ // max idle write timeout parameter.
+ Set<Session> setA = s1a.getOpenSessions();
+ int expected = 1;
+ while (expected > 0) {
+ Assert.assertEquals(expected, getOpenCount(setA));
+ int count = 0;
+ while (getOpenCount(setA) == expected && count < 50) {
+ count ++;
+ Thread.sleep(100);
+ }
+ expected--;
+ }
+ // to make sure the background thread to check expiration runs
+ Thread.sleep(2000);
Review comment:
Generally, sleeping threads to allow a state change to happen result in
intermittent test failures. Better to wait in a loop like the check for the
session to open (which doesn't need to be in such a complex loop).
##########
File path: java/org/apache/tomcat/websocket/WsSession.java
##########
@@ -805,8 +807,13 @@ protected MessageHandler getBinaryMessageHandler() {
}
- protected void updateLastActive() {
+ protected void updateLastActive(boolean isWriteOperation) {
lastActive = System.currentTimeMillis();
+ if (isWriteOperation) {
+ lastWriteTime = lastActive;
+ } else {
+ lastReadTime = lastActive;
+ }
Review comment:
On reflection I decided to split this into two methods for clarity when
reading the calling code.
##########
File path: java/org/apache/tomcat/websocket/WsSession.java
##########
@@ -816,16 +823,62 @@ protected void checkExpiration() {
return;
}
- if (System.currentTimeMillis() - lastActive > timeout) {
+ long currentTime = System.currentTimeMillis();
+
+ if (currentTime - lastActive > timeout) {
String msg = sm.getString("wsSession.timeout", getId());
if (log.isDebugEnabled()) {
log.debug(msg);
}
doClose(new CloseReason(CloseCodes.GOING_AWAY, msg),
new CloseReason(CloseCodes.CLOSED_ABNORMALLY, msg));
+ } else {
+ boolean closeConnection = false;
+ Long idleReadTimeout = getIdleReadTimeout();
+
+ closeConnection = idleReadTimeout != null && (currentTime -
lastReadTime > idleReadTimeout);
+
+ if (closeConnection) {
+ String msg = sm.getString("wsSession.readIdleTimeout",
getId());
+ if (log.isDebugEnabled()) {
+ log.debug(msg);
+ }
+ doClose(new CloseReason(CloseCodes.GOING_AWAY, msg),
+ new CloseReason(CloseCodes.CLOSED_ABNORMALLY, msg));
+ }
+
+ Long idleWriteTimeout = getIdleWriteTimeout();
+
+ closeConnection = idleWriteTimeout != null && (currentTime -
lastWriteTime > idleWriteTimeout);
+
+ if (closeConnection) {
+ String msg = sm.getString("wsSession.writeIdleTimeout",
getId());
+ if (log.isDebugEnabled()) {
+ log.debug(msg);
+ }
+ doClose(new CloseReason(CloseCodes.GOING_AWAY, msg),
+ new CloseReason(CloseCodes.CLOSED_ABNORMALLY, msg));
+ }
Review comment:
Don't need to perform this check if the read timeout has already
triggered. Don't want the possibility of multiple close messages.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
