Hi Mark, I noticed you wrote a websocket test client, I haven't looked at it extensively but I wanted to ask - have you considered using Autobahn for testing? It's rather extensive opensource websocket testing suite. It already contains a lot of stuff we need to test including fragmentation testing.
On Fri, Feb 17, 2012 at 14:50, Mark Thomas <ma...@apache.org> wrote: > On 16/02/2012 04:01, Petr Praus wrote: > > Hello, attached is our patch. It applies cleanly on top of current trunk > > rev. 1244719. It has rudimentary support for fragmentation (callback > > after last frame), supports close messages and ping/pong. Sorry for not > > sending a patchset but I thought it wouldn't really make sense, since > > there were quite a lot of back and forth changes. Let me know if I > > should send a patchset instead. > > The final diff is fine to work with from my point of view. > > > > > (Jonathan's summary) > > "Echoing fragments. Close messages. Pings/pongs. > > > > Just barely works. :) Fragmentation support is very limited: a mere > > callback when last frame received. > > Repeating what I already wrote in BZ on fragmentation so there is a > complete set of review comments in one place > > I am extremely reluctant to apply the current fragmentation patch. It > relies on buffering individual fragments and - given the maximum > fragment size - that is simply not going to scale. This is why the > current API is built on streams and does not buffer unless the message > based API is used. > > I'd like to fully explore the possibility of supporting fragments > without using buffering before starting down that path. > I understand your concern, the main idea was that the logic that pertains only to a single frame and the information about the frame itself should be encapsulated there. What about letting the client code know there's a new frame before we begin reading the data but still keep the frames encapsulated in WebSocketFrame? > > > Sends normal closing reply messages and some protocol error closes. > Answers pings with pongs. > > The fragmentation implementation is likely to impact how this is done, > so I'd like to resolve that issue first and then come back to this. > > > Moving towards a better application API. > > I think I know what you mean by this but please could you explain in > more detail. > > > The servlet container is doing something I don't understand with rapid > connection attempts…not sure what's up. > > Tell us what you observe and we might be able to explain it. > > > I renamed StreamInbound to WebSocketConnection since it's bidirectional. > > This looks to be related to the refactoring. I'm more worried about > design and features than I am naming right now. > > > Also I gave the upgrade processors close() methods. > > That isn't how socket closure needs to be managed. You'll get away with > it with BIO but things are very likely blow up (with a JVM core) when > you do that with APR. You need to let the surrounding Protocol handle > that by returning the correct socket state. > > > Fixed a number of bugs, including > > Please enumerate all the bugs you fixed so I can ensure that part of the > patch is pulled forward ASAP. > > > a switch statement with accidentally cascading cases, > > That looks to be deliberate to me, although a bunch of TODOs wouldn't > have hurt. > This was actually a bug in my code in WebSocketFrame, it wasn't deliberate :) > > and a problem with in Conversions.byteArrayToLong()." > > Thanks. Fixed. > > > General comment: > The code style of a patch needs to be consistent with the Tomcat code > style. Braces on a new line is the obvious thing that jumped out at me > as I read the patch. Actually, this looks to be confined to a single class. > Thanks for the comment, we'll try to keep it consistent. > Yes, I know the Tomcat code base isn't internally consistent style wise. > The code is 10+ years old and it shows. Use the code in the current > websocket package as a basis. > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > >
