You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Steve Huston <sh...@riverace.com> on 2012/11/21 01:54:54 UTC
Review Request: QPID-4424 - prevent multiple threads from processing
PollableQueue events simultaneously
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8147/
-----------------------------------------------------------
Review request for qpid, Andrew Stitcher, Chug Rolke, and Cliff Jansen.
Description
-------
This change prevents multiple threads from processing events at the same time. They can be enqueued by multiple threads but not dispatched.
This addresses bug QPID-4424.
https://issues.apache.org/jira/browse/QPID-4424
Diffs
-----
http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/sys/PollableQueue.h 1411491
Diff: https://reviews.apache.org/r/8147/diff/
Testing
-------
Original test case from QPID-4424.
Thanks,
Steve Huston
Re: Review Request: QPID-4424 - prevent multiple threads from processing
PollableQueue events simultaneously
Posted by Steve Huston <sh...@riverace.com>.
> On Nov. 21, 2012, 2:57 p.m., Andrew Stitcher wrote:
> > This is not an acceptable change to me. The problem is purely a windows one and the assert provides a valuable check for the Unix platform.
> >
> > Basically I think the windows implementation of PollableQueue is just wrong - or rather (given that it is implemented purely as a header file) that the Poller it sits on does not do the correct thing on Windows.
> >
> > Essentially the problem it seems to me is that the code called Poller on Windows does not actually perform the Poller functionality, it merely performs a useful service to the AsynchIO code that sits on top of it. If the name was changed then there would have been no confusion and it couldn't have been used as the basis for the Windows implementation of PollableQueue, which would have needed a different implementation on Windows.
>
> Andrew Stitcher wrote:
> Looking at this a bit more - I think essentially the issue is that PollableCondition (a completely dreadful name which tells you nothing about what the class is supposed to do) needs to ensure that it doesn't dispatch multiple times to PollableQueue.
>
> Cliff Jansen wrote:
> Perhaps there needs to be an equivalent to the completionQueue/working serializing mechanism in windows::AsynchIO here for the PollableConditionPrivate implementation.
>
> I always assumed that this mechanism was there so that the concurrency assumptions could be matched between Linux epoll and the philosophically differing Windows overlapped IO and completion port concepts.
>
> The Linux/Posix poller implementations imposes the one thread per IOHandle rule, whereas the Windows poller is agnostic and leaves it up to the derived IOHandle classes. Presumably all Windows IOHandle classes need this serializing ability, or it needs to be moved into IOHandle or the poller itself.
>
I don't want to revisit the whole IOHandle and AsynchIO thing for this. The issue with PollableCondition and PollableQueue is that there's no IO handle involved - as soon as the PollableCondition poke() queues an IO completion it can be delivered on any thread. I'll work on ensuring that no dispatch occurs while another thread is already dispatching - Andrew's second comment, above.
- Steve
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8147/#review13669
-----------------------------------------------------------
On Nov. 21, 2012, 12:54 a.m., Steve Huston wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8147/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2012, 12:54 a.m.)
>
>
> Review request for qpid, Andrew Stitcher, Chug Rolke, and Cliff Jansen.
>
>
> Description
> -------
>
> This change prevents multiple threads from processing events at the same time. They can be enqueued by multiple threads but not dispatched.
>
>
> This addresses bug QPID-4424.
> https://issues.apache.org/jira/browse/QPID-4424
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/sys/PollableQueue.h 1411491
>
> Diff: https://reviews.apache.org/r/8147/diff/
>
>
> Testing
> -------
>
> Original test case from QPID-4424.
>
>
> Thanks,
>
> Steve Huston
>
>
Re: Review Request: QPID-4424 - prevent multiple threads from processing
PollableQueue events simultaneously
Posted by Cliff Jansen <cl...@gmail.com>.
> On Nov. 21, 2012, 2:57 p.m., Andrew Stitcher wrote:
> > This is not an acceptable change to me. The problem is purely a windows one and the assert provides a valuable check for the Unix platform.
> >
> > Basically I think the windows implementation of PollableQueue is just wrong - or rather (given that it is implemented purely as a header file) that the Poller it sits on does not do the correct thing on Windows.
> >
> > Essentially the problem it seems to me is that the code called Poller on Windows does not actually perform the Poller functionality, it merely performs a useful service to the AsynchIO code that sits on top of it. If the name was changed then there would have been no confusion and it couldn't have been used as the basis for the Windows implementation of PollableQueue, which would have needed a different implementation on Windows.
>
> Andrew Stitcher wrote:
> Looking at this a bit more - I think essentially the issue is that PollableCondition (a completely dreadful name which tells you nothing about what the class is supposed to do) needs to ensure that it doesn't dispatch multiple times to PollableQueue.
Perhaps there needs to be an equivalent to the completionQueue/working serializing mechanism in windows::AsynchIO here for the PollableConditionPrivate implementation.
I always assumed that this mechanism was there so that the concurrency assumptions could be matched between Linux epoll and the philosophically differing Windows overlapped IO and completion port concepts.
The Linux/Posix poller implementations imposes the one thread per IOHandle rule, whereas the Windows poller is agnostic and leaves it up to the derived IOHandle classes. Presumably all Windows IOHandle classes need this serializing ability, or it needs to be moved into IOHandle or the poller itself.
- Cliff
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8147/#review13669
-----------------------------------------------------------
On Nov. 21, 2012, 12:54 a.m., Steve Huston wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8147/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2012, 12:54 a.m.)
>
>
> Review request for qpid, Andrew Stitcher, Chug Rolke, and Cliff Jansen.
>
>
> Description
> -------
>
> This change prevents multiple threads from processing events at the same time. They can be enqueued by multiple threads but not dispatched.
>
>
> This addresses bug QPID-4424.
> https://issues.apache.org/jira/browse/QPID-4424
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/sys/PollableQueue.h 1411491
>
> Diff: https://reviews.apache.org/r/8147/diff/
>
>
> Testing
> -------
>
> Original test case from QPID-4424.
>
>
> Thanks,
>
> Steve Huston
>
>
Re: Review Request: QPID-4424 - prevent multiple threads from processing
PollableQueue events simultaneously
Posted by Andrew Stitcher <as...@apache.org>.
> On Nov. 21, 2012, 2:57 p.m., Andrew Stitcher wrote:
> > This is not an acceptable change to me. The problem is purely a windows one and the assert provides a valuable check for the Unix platform.
> >
> > Basically I think the windows implementation of PollableQueue is just wrong - or rather (given that it is implemented purely as a header file) that the Poller it sits on does not do the correct thing on Windows.
> >
> > Essentially the problem it seems to me is that the code called Poller on Windows does not actually perform the Poller functionality, it merely performs a useful service to the AsynchIO code that sits on top of it. If the name was changed then there would have been no confusion and it couldn't have been used as the basis for the Windows implementation of PollableQueue, which would have needed a different implementation on Windows.
Looking at this a bit more - I think essentially the issue is that PollableCondition (a completely dreadful name which tells you nothing about what the class is supposed to do) needs to ensure that it doesn't dispatch multiple times to PollableQueue.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8147/#review13669
-----------------------------------------------------------
On Nov. 21, 2012, 12:54 a.m., Steve Huston wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8147/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2012, 12:54 a.m.)
>
>
> Review request for qpid, Andrew Stitcher, Chug Rolke, and Cliff Jansen.
>
>
> Description
> -------
>
> This change prevents multiple threads from processing events at the same time. They can be enqueued by multiple threads but not dispatched.
>
>
> This addresses bug QPID-4424.
> https://issues.apache.org/jira/browse/QPID-4424
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/sys/PollableQueue.h 1411491
>
> Diff: https://reviews.apache.org/r/8147/diff/
>
>
> Testing
> -------
>
> Original test case from QPID-4424.
>
>
> Thanks,
>
> Steve Huston
>
>
Re: Review Request: QPID-4424 - prevent multiple threads from processing
PollableQueue events simultaneously
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8147/#review13669
-----------------------------------------------------------
This is not an acceptable change to me. The problem is purely a windows one and the assert provides a valuable check for the Unix platform.
Basically I think the windows implementation of PollableQueue is just wrong - or rather (given that it is implemented purely as a header file) that the Poller it sits on does not do the correct thing on Windows.
Essentially the problem it seems to me is that the code called Poller on Windows does not actually perform the Poller functionality, it merely performs a useful service to the AsynchIO code that sits on top of it. If the name was changed then there would have been no confusion and it couldn't have been used as the basis for the Windows implementation of PollableQueue, which would have needed a different implementation on Windows.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/sys/PollableQueue.h
<https://reviews.apache.org/r/8147/#comment29293>
This comment is entirely untrue for the Unix based ports. There it is not possbible for multiple threads to call dispatch
The underlying assumption of using the Poller mechanism is that once an IOHandle has been returned once from Poller::wait it cannot be returned again until it is re-enabled.
- Andrew Stitcher
On Nov. 21, 2012, 12:54 a.m., Steve Huston wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8147/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2012, 12:54 a.m.)
>
>
> Review request for qpid, Andrew Stitcher, Chug Rolke, and Cliff Jansen.
>
>
> Description
> -------
>
> This change prevents multiple threads from processing events at the same time. They can be enqueued by multiple threads but not dispatched.
>
>
> This addresses bug QPID-4424.
> https://issues.apache.org/jira/browse/QPID-4424
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/sys/PollableQueue.h 1411491
>
> Diff: https://reviews.apache.org/r/8147/diff/
>
>
> Testing
> -------
>
> Original test case from QPID-4424.
>
>
> Thanks,
>
> Steve Huston
>
>
Re: Review Request: QPID-4424 - prevent multiple threads from processing
PollableQueue events simultaneously
Posted by Cliff Jansen <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8147/#review13748
-----------------------------------------------------------
Ship it!
Ship It!
- Cliff Jansen
On Nov. 23, 2012, 11:43 p.m., Steve Huston wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8147/
> -----------------------------------------------------------
>
> (Updated Nov. 23, 2012, 11:43 p.m.)
>
>
> Review request for qpid, Andrew Stitcher, Chug Rolke, and Cliff Jansen.
>
>
> Description
> -------
>
> This change prevents multiple threads from processing events at the same time. They can be enqueued by multiple threads but not dispatched.
>
>
> This addresses bug QPID-4424.
> https://issues.apache.org/jira/browse/QPID-4424
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/sys/windows/PollableCondition.cpp 1412730
>
> Diff: https://reviews.apache.org/r/8147/diff/
>
>
> Testing
> -------
>
> Original test case from QPID-4424.
>
>
> Thanks,
>
> Steve Huston
>
>
Re: Review Request: QPID-4424 - prevent multiple threads from processing
PollableQueue events simultaneously
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8147/#review13701
-----------------------------------------------------------
Ship it!
this second change looks much better to me.
- Andrew Stitcher
On Nov. 23, 2012, 11:43 p.m., Steve Huston wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8147/
> -----------------------------------------------------------
>
> (Updated Nov. 23, 2012, 11:43 p.m.)
>
>
> Review request for qpid, Andrew Stitcher, Chug Rolke, and Cliff Jansen.
>
>
> Description
> -------
>
> This change prevents multiple threads from processing events at the same time. They can be enqueued by multiple threads but not dispatched.
>
>
> This addresses bug QPID-4424.
> https://issues.apache.org/jira/browse/QPID-4424
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/sys/windows/PollableCondition.cpp 1412730
>
> Diff: https://reviews.apache.org/r/8147/diff/
>
>
> Testing
> -------
>
> Original test case from QPID-4424.
>
>
> Thanks,
>
> Steve Huston
>
>
Re: Review Request: QPID-4424 - prevent multiple threads from processing
PollableQueue events simultaneously
Posted by Steve Huston <sh...@riverace.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8147/
-----------------------------------------------------------
(Updated Nov. 23, 2012, 11:43 p.m.)
Review request for qpid, Andrew Stitcher, Chug Rolke, and Cliff Jansen.
Changes
-------
New patch. This one is just for Windows-specific PollableConditionPrivate - it prevents multiple threads from dispatching simultaneously.
Description
-------
This change prevents multiple threads from processing events at the same time. They can be enqueued by multiple threads but not dispatched.
This addresses bug QPID-4424.
https://issues.apache.org/jira/browse/QPID-4424
Diffs (updated)
-----
http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/sys/windows/PollableCondition.cpp 1412730
Diff: https://reviews.apache.org/r/8147/diff/
Testing
-------
Original test case from QPID-4424.
Thanks,
Steve Huston