On 19/06/2012 10:20, Mark Thomas wrote: > On 19/06/2012 06:16, Michael Roberts wrote: >> Hi, I have been playing with the web sockets support in 7.0.27. I have >> some comments and, as requested in the readme, am sending them to the dev >> list. > > Thanks. Much appreciated. > >> Caveat: it's been a while since .27, so perhaps some of these issues >> are addressed in the nightly builds. Anyhow, here we go: > > No worries. 7.0.28 is on the mirrors now if you want to try it after > reading the rest of the my response below. The announcement will go out > later today. > >> 1) In the createWebSocketInbound method implemented by the chat example's >> subclass of WebSocketServlet, there is seemingly no way to interrogate the >> incoming request for information like post/get parms as in a conventional >> servlet. Since these can be sent later via a message, so this is only a >> slight problem. However, a more serious problem is that I can't get at the >> IP address of the incoming request when the connection is being created. >> This means I can't implement something like a block list (common in games) >> to remove annoying things like DOS attacks and misbehaving players from >> hitting my server at the point where a connection is created (and hence >> resources used). Access to some form of parms would allow me to do >> additional validation (like user name and login) at this stage as well. > > Fair point. Would making the request object available be sufficient? It > would almost certainly be wrapped in a façade with the wrapped request > set to null after the method completes to prevent sub-classes retaining > a reference to the request which would create all sorts of problems. > >> 2) MessageInbound is oddly named. Really this is a persistent connection, >> so likely it should likely be named something like WebSocketConnection. >> The chat example hints at this, and maintains a list of "connections", >> which are instances of the subclass of MessageInbound. Since you have >> onTextMessage and onBinaryMessage methods from which to hang actual message >> processing, this rename would be consistent. > > Another fair point. I can't decide whether to change this now or to wait > until JSR 356 is more advanced and change it then. Thoughts? > >> 3) As mentioned, the chat example maintains a list of connections. >> Connections (MessageInbound instances) are added to this list when >> connections are established. However, they are never removed when the >> connection is closed, so the list grows. The broadcast method silently >> ignores the IOException presumably caused by writing to these connections. > > Opps. We'll get that fixed.
I don't see this in the code or when I test it. Could you explain the circumstances in which this happens please? Mark >> 4) I get what look like a timeout (and the connection closes) in case of >> inactivity in the chat example. However, I can't see a way to adjust this, >> or turn the behavior off. See previous comment about the non-removal from >> the list of connections. Previously, in comet, we dealt with things like >> this by doing an immediate reconnect from the javascript side of networking >> code, but it would be nice to have control over this during this time >> around the wheel. > > That was a 'feature' of the first implementation. WebSocket now uses an > infinite time out by default (this required some low level changes to > the APR/native connector which slowed down the 7.0.28 release a little) > and the application can change the time out on a per connection basis. > > Cheers, > > Mark > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
