You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Cliff Jansen <cl...@gmail.com> on 2016/01/22 09:11:31 UTC
Review Request 42647: PROTON-1071: EventInjector hangs on Windows:
add new interrupt primitives to selector and reactor
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42647/
-----------------------------------------------------------
Review request for qpid, Alan Conway, Andrew Stitcher, Gordon Sim, and Justin Ross.
Bugs: PROTON-1071
https://issues.apache.org/jira/browse/PROTON-1071
Repository: qpid-proton-git
Description
-------
Wrote new thread safe primitives for reactor and selector for Windows and adapted the existing posix self-pipe mechanism to the changed API.
Reworked the Python EventInjector to use these primitives via special swig functions.
Updated the io.h documentation to reflect the more conservative thread guarantee.
Diffs
-----
examples/python/db_recv.py 8c79049
examples/python/db_send.py c07dcc0
proton-c/bindings/python/cproton.i 734069f
proton-c/bindings/python/proton/reactor.py 195ff28
proton-c/include/proton/event.h 16d2bda
proton-c/include/proton/io.h 19dfe53
proton-c/include/proton/reactor.h e91b169
proton-c/include/proton/selector.h c942393
proton-c/src/events/event.c 5ad718e
proton-c/src/posix/selector.c 7f72c84
proton-c/src/reactor/reactor.c 7ea279b
proton-c/src/windows/iocp.h 0e052e5
proton-c/src/windows/iocp.c 404dd36
proton-c/src/windows/selector.c f139aec
tests/python/proton_tests/reactor.py 6ee107d
Diff: https://reviews.apache.org/r/42647/diff/
Testing
-------
rhel7, windows8
Thanks,
Cliff Jansen
Re: Review Request 42647: PROTON-1071: EventInjector hangs on
Windows: add new interrupt primitives to selector and reactor
Posted by Alan Conway <ac...@redhat.com>.
> On Jan. 25, 2016, 2:42 p.m., Gordon Sim wrote:
> > proton-c/bindings/python/cproton.i, line 447
> > <https://reviews.apache.org/r/42647/diff/1/?file=1205459#file1205459line447>
> >
> > 'async_notify' is a horrible name... is this in addition to or instead of on_interrupt?
I'm ok with this as a reactor hack provided we are not going to add any events to the messaging handler. If we have to add an event to the low-level proton handler it should be `on_reactor_foo` so its clear this is reactor-only. IMO this is a perfect example of the problem with the reactor design. The right way to do interrupts and injection across threads is very platform specific. The pipe trick is great on posix, but you could handle this much more easily on Windows if you weren't stuck with the posix-inpired design. In Go this is all nonsense, you pass functions on a channel. In python and ruby I'd expect to pass callable objects not just dead "events", but in both cases using C to intermediate will cause problems with the GIL/GVL locks. This stuff is just much easier if you use the tools in the language at hand. I will have ruby and C++ injection going shortly around the connection_engine, it already is working in Go if you want to check it out.
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42647/#review116070
-----------------------------------------------------------
On Jan. 22, 2016, 8:11 a.m., Cliff Jansen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42647/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2016, 8:11 a.m.)
>
>
> Review request for qpid, Alan Conway, Andrew Stitcher, Gordon Sim, and Justin Ross.
>
>
> Bugs: PROTON-1071
> https://issues.apache.org/jira/browse/PROTON-1071
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Wrote new thread safe primitives for reactor and selector for Windows and adapted the existing posix self-pipe mechanism to the changed API.
>
> Reworked the Python EventInjector to use these primitives via special swig functions.
>
> Updated the io.h documentation to reflect the more conservative thread guarantee.
>
>
> Diffs
> -----
>
> examples/python/db_recv.py 8c79049
> examples/python/db_send.py c07dcc0
> proton-c/bindings/python/cproton.i 734069f
> proton-c/bindings/python/proton/reactor.py 195ff28
> proton-c/include/proton/event.h 16d2bda
> proton-c/include/proton/io.h 19dfe53
> proton-c/include/proton/reactor.h e91b169
> proton-c/include/proton/selector.h c942393
> proton-c/src/events/event.c 5ad718e
> proton-c/src/posix/selector.c 7f72c84
> proton-c/src/reactor/reactor.c 7ea279b
> proton-c/src/windows/iocp.h 0e052e5
> proton-c/src/windows/iocp.c 404dd36
> proton-c/src/windows/selector.c f139aec
> tests/python/proton_tests/reactor.py 6ee107d
>
> Diff: https://reviews.apache.org/r/42647/diff/
>
>
> Testing
> -------
>
> rhel7, windows8
>
>
> Thanks,
>
> Cliff Jansen
>
>
Re: Review Request 42647: PROTON-1071: EventInjector hangs on
Windows: add new interrupt primitives to selector and reactor
Posted by Alan Conway <ac...@redhat.com>.
> On Jan. 25, 2016, 2:42 p.m., Gordon Sim wrote:
> > proton-c/src/events/event.c, line 140
> > <https://reviews.apache.org/r/42647/diff/1/?file=1205465#file1205465line140>
> >
> > It would be worth a comment here explaining why PN_INTERRUPT is a special case.
PN_REACTOR_INTERRUPT please!
> On Jan. 25, 2016, 2:42 p.m., Gordon Sim wrote:
> > proton-c/src/events/event.c, line 385
> > <https://reviews.apache.org/r/42647/diff/1/?file=1205465#file1205465line385>
> >
> > I'd be inclined to make this a reactor scoped event i.e. PN_REACTOR_INTERRUPT
I should read ahead.
> On Jan. 25, 2016, 2:42 p.m., Gordon Sim wrote:
> > proton-c/src/posix/selector.c, line 68
> > <https://reviews.apache.org/r/42647/diff/1/?file=1205466#file1205466line68>
> >
> > Would be safer to set the fds to -1 after closing.
INVALID_SOCKET on windows, -1 is a posixism.
On Jan. 25, 2016, 2:42 p.m., Cliff Jansen wrote:
> > Since the interrupt is now essentially a special method on the reactor, EventInjector as a separate, user-visiable class doesn't make as much sense in my view. I'd be inclined to try and merge it into Container.
I am confused by the event injector. Why is it injecting a proton event? What event type does it have? I would have expected to simply inject a callable object that will be executed in the correct context, or at least an event with a specific APPLICATION_EVENT type to which I could attach my own code or data. Why would I disguise an application event as if it came from proton?
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42647/#review116070
-----------------------------------------------------------
On Jan. 22, 2016, 8:11 a.m., Cliff Jansen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42647/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2016, 8:11 a.m.)
>
>
> Review request for qpid, Alan Conway, Andrew Stitcher, Gordon Sim, and Justin Ross.
>
>
> Bugs: PROTON-1071
> https://issues.apache.org/jira/browse/PROTON-1071
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Wrote new thread safe primitives for reactor and selector for Windows and adapted the existing posix self-pipe mechanism to the changed API.
>
> Reworked the Python EventInjector to use these primitives via special swig functions.
>
> Updated the io.h documentation to reflect the more conservative thread guarantee.
>
>
> Diffs
> -----
>
> examples/python/db_recv.py 8c79049
> examples/python/db_send.py c07dcc0
> proton-c/bindings/python/cproton.i 734069f
> proton-c/bindings/python/proton/reactor.py 195ff28
> proton-c/include/proton/event.h 16d2bda
> proton-c/include/proton/io.h 19dfe53
> proton-c/include/proton/reactor.h e91b169
> proton-c/include/proton/selector.h c942393
> proton-c/src/events/event.c 5ad718e
> proton-c/src/posix/selector.c 7f72c84
> proton-c/src/reactor/reactor.c 7ea279b
> proton-c/src/windows/iocp.h 0e052e5
> proton-c/src/windows/iocp.c 404dd36
> proton-c/src/windows/selector.c f139aec
> tests/python/proton_tests/reactor.py 6ee107d
>
> Diff: https://reviews.apache.org/r/42647/diff/
>
>
> Testing
> -------
>
> rhel7, windows8
>
>
> Thanks,
>
> Cliff Jansen
>
>
Re: Review Request 42647: PROTON-1071: EventInjector hangs on
Windows: add new interrupt primitives to selector and reactor
Posted by Gordon Sim <gs...@redhat.com>.
On Jan. 25, 2016, 2:42 p.m., Cliff Jansen wrote:
> > Since the interrupt is now essentially a special method on the reactor, EventInjector as a separate, user-visiable class doesn't make as much sense in my view. I'd be inclined to try and merge it into Container.
>
> Alan Conway wrote:
> I am confused by the event injector. Why is it injecting a proton event? What event type does it have? I would have expected to simply inject a callable object that will be executed in the correct context, or at least an event with a specific APPLICATION_EVENT type to which I could attach my own code or data. Why would I disguise an application event as if it came from proton?
It is indeed application specific events types that are injected (and they can have their own context). They aren't intended to be 'disguised' in anyway, but they have to confrm to the collectors notion of what an event is. The choice of events as opposed to callable objects was an attempt at simplicity through more uniformity.
- Gordon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42647/#review116070
-----------------------------------------------------------
On Jan. 22, 2016, 8:11 a.m., Cliff Jansen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42647/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2016, 8:11 a.m.)
>
>
> Review request for qpid, Alan Conway, Andrew Stitcher, Gordon Sim, and Justin Ross.
>
>
> Bugs: PROTON-1071
> https://issues.apache.org/jira/browse/PROTON-1071
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Wrote new thread safe primitives for reactor and selector for Windows and adapted the existing posix self-pipe mechanism to the changed API.
>
> Reworked the Python EventInjector to use these primitives via special swig functions.
>
> Updated the io.h documentation to reflect the more conservative thread guarantee.
>
>
> Diffs
> -----
>
> examples/python/db_recv.py 8c79049
> examples/python/db_send.py c07dcc0
> proton-c/bindings/python/cproton.i 734069f
> proton-c/bindings/python/proton/reactor.py 195ff28
> proton-c/include/proton/event.h 16d2bda
> proton-c/include/proton/io.h 19dfe53
> proton-c/include/proton/reactor.h e91b169
> proton-c/include/proton/selector.h c942393
> proton-c/src/events/event.c 5ad718e
> proton-c/src/posix/selector.c 7f72c84
> proton-c/src/reactor/reactor.c 7ea279b
> proton-c/src/windows/iocp.h 0e052e5
> proton-c/src/windows/iocp.c 404dd36
> proton-c/src/windows/selector.c f139aec
> tests/python/proton_tests/reactor.py 6ee107d
>
> Diff: https://reviews.apache.org/r/42647/diff/
>
>
> Testing
> -------
>
> rhel7, windows8
>
>
> Thanks,
>
> Cliff Jansen
>
>
Re: Review Request 42647: PROTON-1071: EventInjector hangs on
Windows: add new interrupt primitives to selector and reactor
Posted by Alan Conway <ac...@redhat.com>.
On Jan. 25, 2016, 2:42 p.m., Cliff Jansen wrote:
> > Since the interrupt is now essentially a special method on the reactor, EventInjector as a separate, user-visiable class doesn't make as much sense in my view. I'd be inclined to try and merge it into Container.
>
> Alan Conway wrote:
> I am confused by the event injector. Why is it injecting a proton event? What event type does it have? I would have expected to simply inject a callable object that will be executed in the correct context, or at least an event with a specific APPLICATION_EVENT type to which I could attach my own code or data. Why would I disguise an application event as if it came from proton?
>
> Gordon Sim wrote:
> It is indeed application specific events types that are injected (and they can have their own context). They aren't intended to be 'disguised' in anyway, but they have to confrm to the collectors notion of what an event is. The choice of events as opposed to callable objects was an attempt at simplicity through more uniformity.
YOu're right I misread.
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42647/#review116070
-----------------------------------------------------------
On Jan. 22, 2016, 8:11 a.m., Cliff Jansen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42647/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2016, 8:11 a.m.)
>
>
> Review request for qpid, Alan Conway, Andrew Stitcher, Gordon Sim, and Justin Ross.
>
>
> Bugs: PROTON-1071
> https://issues.apache.org/jira/browse/PROTON-1071
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Wrote new thread safe primitives for reactor and selector for Windows and adapted the existing posix self-pipe mechanism to the changed API.
>
> Reworked the Python EventInjector to use these primitives via special swig functions.
>
> Updated the io.h documentation to reflect the more conservative thread guarantee.
>
>
> Diffs
> -----
>
> examples/python/db_recv.py 8c79049
> examples/python/db_send.py c07dcc0
> proton-c/bindings/python/cproton.i 734069f
> proton-c/bindings/python/proton/reactor.py 195ff28
> proton-c/include/proton/event.h 16d2bda
> proton-c/include/proton/io.h 19dfe53
> proton-c/include/proton/reactor.h e91b169
> proton-c/include/proton/selector.h c942393
> proton-c/src/events/event.c 5ad718e
> proton-c/src/posix/selector.c 7f72c84
> proton-c/src/reactor/reactor.c 7ea279b
> proton-c/src/windows/iocp.h 0e052e5
> proton-c/src/windows/iocp.c 404dd36
> proton-c/src/windows/selector.c f139aec
> tests/python/proton_tests/reactor.py 6ee107d
>
> Diff: https://reviews.apache.org/r/42647/diff/
>
>
> Testing
> -------
>
> rhel7, windows8
>
>
> Thanks,
>
> Cliff Jansen
>
>
Re: Review Request 42647: PROTON-1071: EventInjector hangs on
Windows: add new interrupt primitives to selector and reactor
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42647/#review116070
-----------------------------------------------------------
proton-c/bindings/python/cproton.i (line 409)
<https://reviews.apache.org/r/42647/#comment177087>
I'm confused by the comment... Can we remove this and just have interrupt() on the Reactor?
proton-c/bindings/python/cproton.i (line 447)
<https://reviews.apache.org/r/42647/#comment177088>
'async_notify' is a horrible name... is this in addition to or instead of on_interrupt?
proton-c/bindings/python/proton/reactor.py (line 232)
<https://reviews.apache.org/r/42647/#comment177089>
In the modified code, what is _closed ever used for?
proton-c/include/proton/reactor.h (line 90)
<https://reviews.apache.org/r/42647/#comment177090>
Do we need to be able to pass in a handler? Why not just use whatever handler the reactor si configured with?
proton-c/src/events/event.c (line 140)
<https://reviews.apache.org/r/42647/#comment177091>
It would be worth a comment here explaining why PN_INTERRUPT is a special case.
proton-c/src/events/event.c (line 385)
<https://reviews.apache.org/r/42647/#comment177092>
I'd be inclined to make this a reactor scoped event i.e. PN_REACTOR_INTERRUPT
proton-c/src/posix/selector.c (line 68)
<https://reviews.apache.org/r/42647/#comment177080>
Would be safer to set the fds to -1 after closing.
proton-c/src/posix/selector.c (line 82)
<https://reviews.apache.org/r/42647/#comment177098>
Does this mean we are creating a pipe not just for each reactor but for every connection within it? That doesn't seem right if so...
Since the interrupt is now essentially a special method on the reactor, EventInjector as a separate, user-visiable class doesn't make as much sense in my view. I'd be inclined to try and merge it into Container.
- Gordon Sim
On Jan. 22, 2016, 8:11 a.m., Cliff Jansen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42647/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2016, 8:11 a.m.)
>
>
> Review request for qpid, Alan Conway, Andrew Stitcher, Gordon Sim, and Justin Ross.
>
>
> Bugs: PROTON-1071
> https://issues.apache.org/jira/browse/PROTON-1071
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Wrote new thread safe primitives for reactor and selector for Windows and adapted the existing posix self-pipe mechanism to the changed API.
>
> Reworked the Python EventInjector to use these primitives via special swig functions.
>
> Updated the io.h documentation to reflect the more conservative thread guarantee.
>
>
> Diffs
> -----
>
> examples/python/db_recv.py 8c79049
> examples/python/db_send.py c07dcc0
> proton-c/bindings/python/cproton.i 734069f
> proton-c/bindings/python/proton/reactor.py 195ff28
> proton-c/include/proton/event.h 16d2bda
> proton-c/include/proton/io.h 19dfe53
> proton-c/include/proton/reactor.h e91b169
> proton-c/include/proton/selector.h c942393
> proton-c/src/events/event.c 5ad718e
> proton-c/src/posix/selector.c 7f72c84
> proton-c/src/reactor/reactor.c 7ea279b
> proton-c/src/windows/iocp.h 0e052e5
> proton-c/src/windows/iocp.c 404dd36
> proton-c/src/windows/selector.c f139aec
> tests/python/proton_tests/reactor.py 6ee107d
>
> Diff: https://reviews.apache.org/r/42647/diff/
>
>
> Testing
> -------
>
> rhel7, windows8
>
>
> Thanks,
>
> Cliff Jansen
>
>