Github user gemmellr commented on the pull request:
https://github.com/apache/qpid-proton/pull/71#issuecomment-198459041
Hi Zoltan,
Sorry for the delay, I have finally given this a look, albeit a relatively
quick one. I havenât spent as long looking at it as I might like to (so I
might have completely misunderstood some things), and I havenât tried it out,
but Iâm off on vacation for the next week-and-a-bit and wanted to comment
before I disappear.
My initial reaction was that Iâm not sure I like the idea of the core
engine Transport having more things to do that arenât really about AMQP
directly, but more IO. On the other hand I guess this way lets it works across
different IO / API models, such as that imposed by the existing Reactor code,
and it would be optional so folks wouldnât need to use it if they have a
separate IO layer to do this. Probably something we should discuss in the
community.
Setting that aside that for now, I had some more code-specific comments
from my initial look though:
- Silently skipping doing anything WebSockets if the âconfigure/initâ
step is missed out doesnât seem very nice. If folks call the websocket()
method then I think that is what they should actually get (or some form of
error upon use, if any further necessary config isnât then provided). Doing
away with the âisEnabledâ stuff would seem to simplify things elsewhere too.
- Related to above, the reactor io handler always calling
transport.websocket() seems an odd choice. It wonât do much if not further
configured, so it seems whoever is ultimately configuring it (example would
help here) could request the websocket use originally too, in fact presumably
they would have to in order to get the object to configure it. Also WebSocket
webSocket = transport.webSocket(); creates an unused variable in the reactor.
- The configure method isnât actually exposed on the interface to let it
be called anyway?
- The tracking of the _webSocketHeaderSize and its use in pop seemed frail,
if someone calls head()/pending() more than once or pops less than the total
pending at each use it seemed like it could end up popping the wrong amount
from the underlying buffer.
- WebSocketHandlerImpl.unwrapBuffer(ByteBuffer) has some unused variables.
It also seems to make some questionable returns. E.g if there arenât enough
bytes (yet..they may still be coming) to determine a size, it returns
âinvalid lengthâ, and since the
WebSocketImpl.WebSocketTransportWrapper.processInput() method seems to treat
most return values as simply âpour the websocket input buffer into the
underlying inputâ, it would then seem it could do the wrong thing in such
cases. unwrapBuffer also doesnât seem to do anything with the actual lengths
it calculates.
- The above makes it seem like like it can only process 1 frame each time
process is called, and assuming only a single websocket frames content will be
present in the buffer and the start of the buffer is always the header. Is that
the case, or did I miss something important? Those seem like assumptions that
donât necessarily hold, and could give unexpected behaviour.
- The Websocket impl âmax frame sizeâ isnât configurable and seems a
little arbitrary, but overlooking that it doesnt seem like the handler will
cope with an underlying buffer having more output than it can fit, either due
to a single larger frame, or the combination of multiple frames awaiting
transmission. The OOME thrown in that case is perhaps misleading (given there
is still memory, just not enough output buffer space, i.e another exception
type might be better), though I think it really just shouldnât throw an
exception and rather send what it can.
- The âclient-onlyâ impl detail mentioned here is not clear in the
code. Could use some doc, or maybe config? This is also a little unfortunate
since everything else in the Transport works at both ends of a connection.
- It isnât clear why there a âWebsocketSnifferâ if the impl is
client-only, a sniffer would be used at a server end normally. Also, the only
non-test usage of it (in WebSocketImpl#wrap) seems strange in that it overrides
any choice anyway.
P.S. please rebase Pull Requests against the current master to remove merge
commits and related noise from them.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
