Github user gemmellr commented on the pull request:
https://github.com/apache/qpid-proton/pull/71#issuecomment-198597691
HI Zoltan,
Some more replies (inline) to your replies before I disappear for the week
:)
On 18 March 2016 at 21:14, Zoltan Varga <notificati...@github.com> wrote:
> Hi Rob,
>
> Thanks for the quick response. See answers/questions inlineâ¦
>
> Cheers,
> Zoltan
>
> From: Robbie Gemmell [mailto:notificati...@github.com]
> Sent: Friday, March 18, 2016 10:15 AM
> To: apache/qpid-proton <qpid-pro...@noreply.github.com>
> Cc: Zoltan Varga <zolva...@microsoft.com>
> Subject: Re: [qpid-proton] Adding WebSocket functionality to Proton-J
> (#71)
>
>
> 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.
>
> [Zoltan] I agree, the design would be cleaner to separate the WebSocket IO
> from the AMQP IO. I have tried to design it that way, but I could not find
> a clean entry point in the reactor architecture where an âexternalâ
> WebSocket implementation would handle the IO buffer. The reactor currently
> doesnât provide IO independent events for modify the IO buffer before
send
> and after receive. We would need an interface here which communicates
> through input/output buffers only, completely separated from the
underlying
> channel. If that kind of communication with reactor would be possible it
> would allow us to use external WebSocket library. In that case the
external
> library would do all the IO (Socket, SSL engine etc.) and Proton-J would
> just handle a buffer regarding the AMQP bits. I donât think Proton-J
> reactor is prepared to do that but correct me if I am wrong. Please let me
> know if I missed something here.
>
[Robbie] It looks like the only thing the reactor currently lets you do in
that regard is replace its IOHandler (via setGlobalHandler), which one
could use to provide a custom handler that did the input/output bits
differently, but there doesn't appear to be anything particularly precise
beyond that. The core engine (which is what proton was conceived as, and is
how proton-j still sees most of its use, e.g in the JMS client or ActiveMQ
brokers) Transport does already work on a purely input/output buffer
interface, its just you just hook everything else up around it yourself if
used that way. The reactor is but one implementation of wrapper for the
engine which handles IO, satisfies the single-threaded use required by the
engine, and tries to make it easier to use (but unfortunately isn't very
nicely separated), there isn't actually any requirement to use it. The Java
reactor was a port someone did of the proton-c reactor, but none of the
other higher level reactive API work that has been done in the
Python/C++/etc bindings for has yet happened for the Java bits. Its my hope
to start on similar bits for proton-j once I get some other stuff out the
way (after my vacation..). One of the things that the newer C++ work
includes is the idea of a 'connection engine' that mostly separates the IO
handling from the rest, making it more easily used by other components (e.g
perhaps even the reactor itself could) in the style you outlined, its
likely I'll look to do something similar.
>
> 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.
> [Zoltan] I see your point. The current WebSocket initialization
> implementation is following the implementation of the other layers (like
> SASL). This is basically the consequence of the design choice to integrate
> WebSocket IO into the reactor. We can discuss alternative approach if we
> decided on the integration question you asked.
>
> [Robbie] The SASL layer for example starts to do something as soon as you
make the sasl() call that creates it, e.g altering the header output and
swapping in the SASL frame parser etc, which is the key thing for me. As
soon as you call websocket(), it should be doing something, even if thats
only to fail if further requried config isnt provided before it gets used.
> * 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.
> [Zoltan] See the answer above. I know about the unused variable, I will
> remove it.
>
> * The configure method isnât actually exposed on the interface to let it
> be called anyway?
> [Zoltan] Good catch. I am going to change it.
>
> * 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.
> [Zoltan] I am going to revisit the implementations point above.
>
> * 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.
> [Zoltan] Yes, that is the case but I think assumption here is correct.
> Even if WebSocket frames contain partial AMQP messages the WebSocket frame
> will be always complete.
>
[Robbie] As I say, I may have overlooked things in my quick look, but I
didn't spot how those things are ensured. What prevents the buffer
containing more than one websocket frame if it is large enough and the IO
read returned the content? Similarly, if the buffer/read was not large
enough for the entirety of an in-progress frame to be put in it, wouldnt
the next call provide data from the middle of a websocket frame?
>
> * 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.
> [Zoltan] I am going to revisit this too.
>
> * 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.
> [Zoltan] Yes, the server implementation is missing and it is unfortunate
> but this is what we have currently. This would serve our scenarios and we
> were focusing only client side implementation.
>
> * 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.
> [Zoltan] I agree, if we support client implementation only.
>
> P.S. please rebase Pull Requests against the current master to remove
> merge commits and related noise from them.
>
> [Zoltan] Sure.
>
> â
> You are receiving this because you authored the thread.
> Reply to this email directly or view it on GitHub<
> https://github.com/apache/qpid-proton/pull/71#issuecomment-198459041>
>
> â
> You are receiving this because you commented.
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/qpid-proton/pull/71#issuecomment-198544346>
>
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
