Re: Creating a sender outside of a container's thread

[Mark Banner]

I have gone through the documentation and code and have the following
understanding of container vs connection_engine:

Container is an adapter for pn_reactor. The proton reactor manages network
IO and sends events to a registered handler so container registers an proxy
handler which will pass events to the user's implementation.

Connection_engine passes IO to a pn_transport (through io_read and
io_write) and receives events through a pn_collector. Dispatching is then
done directly to the user handler.

>From my understanding connection_engine looks more efficient than the
container for the same functionality (if socket_engine is used). It also
offers more control since a custom implementation can modify the event loop
and mock IO for testing. This makes it a better fit for me compared to the
proposed "application event injection" which is proposed in the thread on
qpid-users.
Am I missing something which is managed by the reactor or the container?

Also, the release notes for proton 12 say that the API for container is
stable but that connection_engine is still unstable. Will this interface
still have the same role in future versions (ie converting IO to proton
events) ?

Thanks for your help
Mark

On Fri, Mar 18, 2016 at 11:03 AM, Mark Banner  wrote:

> I'll look into the connection_engine API then. Thanks for the reply!
>
> On Thu, Mar 17, 2016 at 9:15 PM, Alan Conway  wrote:
>
>> On Thu, 2016-03-17 at 20:02 +0100, Mark Banner wrote:
>> > Hi,
>> >
>> > I am creating an application which is using the C++ API for AMQP
>> > (0.12) and
>> > I am trying to wrap my head around how to create a sender when I have
>> > a new
>> > message to send.
>> >
>> > For receiving messages, I can create a receiver when the container is
>> > finished initializing and on_message will be called whenever a new
>> > message
>> > is received. However, for sending messages, if I create a sender when
>> > the
>> > reactor initializes, on_sendable will be called when it is not
>> > needed. From
>> > what I understand in the examples, the a sender should be closed when
>> > all
>> > the client's messages have been sent.
>> >
>> > How should I go about telling the container I want to send a message?
>> > Is it
>> > safe to pass the connection which is set up in on_start outside of
>> > the
>> > container and to create a new sender in another thread?
>> >
>> > Any help would be much appreciated,
>> > Mark
>>
>> The container is not thread safe, and it is not safe to use any
>> container-owned proton objects outside the thread that calls
>> container::run(). Presently there is no way to stop it running. This is
>> a very serious limitation.
>>
>> There is a plan to add "application event injection" which would allow
>> you to inject custom actions from arbitrary threads and have them
>> executed safely in the container::run() thread. I expect that to
>> materialize soon.
>>
>> There is also an alternative to the container: the connection_engine.
>> It gives you full control over how IO and threads interact with proton.
>> Right now it is raw but usable (see the cpp/engine examples.) It is my
>> top priority to make this easy to use for all the common multi-threaded
>> use cases. I would be happy to help you use it and to be guided by your
>> use cases in developing it for the future.
>>
>> Cheers,
>> Alan.
>>
>>
>

[GitHub] qpid-proton pull request: Adding WebSocket functionality to Proton...

[gemmellr]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] qpid-proton pull request: Adding WebSocket functionality to Proton...

[zolvarga]

Github user zolvarga commented on the pull request:

https://github.com/apache/qpid-proton/pull/71#issuecomment-198544346
  
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 
Cc: Zoltan Varga 
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.

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.

  *   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