You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Bob DeRemer <bo...@thingworx.com> on 2013/10/27 22:14:21 UTC
[potential?] bug in Tomcat 7's websocket UpgradeUtil when handling
concurrent connections that use a custom ServerEndpointConfig.Configurator
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.
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
Map<String, List<String>> 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
RE: [potential?] bug in Tomcat 7's websocket UpgradeUtil when
handling concurrent connections that use a custom
ServerEndpointConfig.Configurator
Posted by Bob DeRemer <bo...@thingworx.com>.
> -----Original Message-----
> From: Mark Thomas [mailto:markt@apache.org]
> Sent: Monday, October 28, 2013 8:53 AM
> To: Tomcat Developers List
> Subject: Re: [potential?] bug in Tomcat 7's websocket UpgradeUtil when
> handling concurrent connections that use a custom
> ServerEndpointConfig.Configurator
>
> On 28/10/2013 12:18, Mark Thomas wrote:
>
> > I'm going to start work on updating Tomcat just as soon as I finish
> > typing this.
>
> Try now. I've applied to fix to both 8.0.x and 7.0.x for the next release of each.
>
> Mark
>
Thanks for the quick turnaround
-bob
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org For additional
> commands, e-mail: dev-help@tomcat.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [potential?] bug in Tomcat 7's websocket UpgradeUtil when handling
concurrent connections that use a custom ServerEndpointConfig.Configurator
Posted by Mark Thomas <ma...@apache.org>.
On 28/10/2013 12:18, Mark Thomas wrote:
> I'm going to start work on updating Tomcat just as soon as I finish
> typing this.
Try now. I've applied to fix to both 8.0.x and 7.0.x for the next
release of each.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [potential?] bug in Tomcat 7's websocket UpgradeUtil when handling
concurrent connections that use a custom ServerEndpointConfig.Configurator
Posted by Mark Thomas <ma...@apache.org>.
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.
I also opened https://java.net/jira/browse/WEBSOCKET_SPEC-215
> 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.
It isn't a bug. It is a bug / grey area / whatever in the spec. If you
do create this, please also include the reference to WEBSOCKET_SPEC-215.
I'm going to start work on updating Tomcat just as soon as I finish
typing this.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [potential?] bug in Tomcat 7's websocket UpgradeUtil when handling
concurrent connections that use a custom ServerEndpointConfig.Configurator
Posted by Mark Thomas <ma...@apache.org>.
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
>
> Map<String, List<String>> 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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org