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. > 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. > 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. 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
