subject:"is this bug in tomcat 7 \?"

Re: [potential?] bug in Tomcat 7's websocket UpgradeUtil when handling concurrent connections that use a custom ServerEndpointConfig.Configurator

2013-10-28 Thread Mark Thomas

On 27/10/2013 21:14, Bob DeRemer wrote:
 First off, this is the 2nd [potential] bug I've reposted from the
 user list, so my apologies.  After reading the mailing list
 descriptions, it seems potential bugs should be posted to the DEV
 list.  If this is wrong, just let me know.

Generally, if you are unsure post to the users list first. If it needs
to move to the dev list, someone will say so. There is often merit in
discussing it on the users list anyway as it givers other folks a chance
to say me too and add their information to the discussion.

When a users list discussion reaches the point where folks are fairly
sure there is a bug then a Bugzilla entry can be created. Often you'll
see a committer ask someone to create one as a result of a discussion.

Posts like this to the dev list are fine but there is a risk (unlikely
in this case) that the report gets forgotten about if a Bugzilla entry
isn't created.

I haven't looked into the detail of this problem - that is next on my
TODO list - but I wanted to address the Is it OK to do this? aspect first.

Mark

 Potential Bug?
 
 
 
 
 
 I [think] there may be a problem in Tomcat's static
 UpgradeUtil.doUpgrade logic when handling concurrent
 connection/upgrade requests that rely on a custom
 ServerEndpointConfig.Configurator.modifyHandshake to grab
 [per-upgrade-request] client header values and inject them into the
 wsSession that is being created.
 
 
 
 Specifically, the static doUpgrade does not appear to make a copy of
 the ServerEndpointConfig before calling modifyHandshake.   As a
 result, any per-connection headers the Configurator may grab and put
 in the ServerEndpointConfig.UserProperties map will be overwritten by
 the last upgrade request that occurs before the upgrade logic creates
 the new wsSession in the WsHttpUpgradeHandler.init call.
 
 
 
 I am able to replicate this very easily by using the following server
 configurator code.  By making concurrent websocket connect requests
 that place a unique client-id in the upgrade request headers, then
 grabbing that client-id property using the code below, ALL
 websocket sessions that get created will have the last client-id
 header value that came in concurrently.
 
 
 
 public class Jsr356ServerConfigurator extends
 ServerEndpointConfig.Configurator {
 
 @Override
 
 public void modifyHandshake(ServerEndpointConfig config,
 HandshakeRequest request, HandshakeResponse response)
 
 {
 
 // get the request headers - looking for security claims and endpoint
 ID
 
 // claims will be checked in the OnOpen call and the connection
 closed if AUTH check fails
 
 MapString, ListString headers = request.getHeaders();
 
 
 
 String id = headers.get(client-id).get(0);
 
 
 
 config.getUserProperties().put(client-id, id);
 
 }
 
 }
 
 
 
 @ServerEndpoint(...)
 
 Public class MyServer
 
 {
 
 @OnOpen
 
 public void onOpen(Session session)
 
 {
 
 String id = (String) session.getUserProperties().get(client-id);
 
 _logger.warn(client ID: {}, id);
 
 }
 
 }
 
 
 
 
 
 Based on chapter 3 of the JSR-356 API document, the actual websocket
 handshake process defined in the websocket spec,  and the online
 description of the process in this stackoverflow link
 (http://stackoverflow.com/questions/17936440/accessing-httpsession-from-httpservletrequest-in-a-web-socket-socketendpoint/17994303#17994303),
 it appears that we should be able to pass per-client information in
 the upgrade headers and we should be able to get them into the
 endpoint INSTANCE's Session user properties.
 
 
 
 If this is a bug, please confirm and I will create a bugzilla entry,
 as it is very important that that we be able to do what I've
 described above.
 
 
 
 If this is not a bug, can someone please describe how to obtain
 per-client instance data during the websocket handshake/onOpen
 process?
 
 
 
 
 
 Thanks,
 
 Bob
 
 


-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: [potential?] bug in Tomcat 7's websocket UpgradeUtil when handling concurrent connections that use a custom ServerEndpointConfig.Configurator

2013-10-28 Thread Mark Thomas

On 27/10/2013 21:14, Bob DeRemer wrote:
Potential Bug?

I [think] there may be a problem in Tomcat's static
UpgradeUtil.doUpgrade logic when handling concurrent
connection/upgrade requests that rely on a custom
ServerEndpointConfig.Configurator.modifyHandshake to grab
[per-upgrade-request] client header values and inject them into the
wsSession that is being created.

Specifically, the static doUpgrade does not appear to make a copy of
the ServerEndpointConfig before calling modifyHandshake. As a
result, any per-connection headers the Configurator may grab and put
in the ServerEndpointConfig.UserProperties map will be overwritten by
the last upgrade request that occurs before the upgrade logic creates
the new wsSession in the WsHttpUpgradeHandler.init call.

I've looked through the specification, the Javadoc and the EG archives
and I can't find any justification for the copying of
ServerEndpointConfig.userProperties to Session.userProperties

I am able to replicate this very easily by using the following server
configurator code. By making concurrent websocket connect requests
that place a unique client-id in the upgrade request headers, then
grabbing that client-id property using the code below, ALL
websocket sessions that get created will have the last client-id
header value that came in concurrently.

snip/

Based on chapter 3 of the JSR-356 API document, the actual websocket
handshake process defined in the websocket spec,

Hmm. I don't see any justification for the current behaviour in these
documents

and the online
description of the process in this stackoverflow link
(http://stackoverflow.com/questions/17936440/accessing-httpsession-from-httpservletrequest-in-a-web-socket-socketendpoint/17994303#17994303),
it appears that we should be able to pass per-client information in
the upgrade headers and we should be able to get them into the
endpoint INSTANCE's Session user properties.

I don't see any justification for this position either. However, I do
think that it is correct. I'll update Tomcat's implementation to align
with Jetty's.

38 matches

Mail list logo