Qpid
  1. Qpid
  2. QPID-3759

Heartbeat timeout in Windows does not lead to timely reconnect

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.14
    • Fix Version/s: 0.17
    • Component/s: C++ Client
    • Labels:
      None
    • Environment:

      Windows C++ messaging

      Description

      Reported by Wolf Wolfswinkel on Qpid users http://qpid.2158936.n2.nabble.com/Heartbeats-in-C-broker-on-Windows-td7118702.html 22-Dec-2011

      The simplest test case is in attached main.cpp. Establish a good network connection to the broker and then start the program. It creates a connection, sends two messages, and then pauses for 15 seconds. During the pause disconnect the network connection to the broker for at least two heartbeat timeouts (12 seconds).

      After the heartbeat timeout the timer task fires and a debug trace shows:
      Traffic timeout, TCPConnector::abort, TCPConnector::eof, TCPConnector::close

      But the connection is not actually closed until something happens on the network to wake up the thread waiting in Poller::run().

      The timer event appears unable to interrupt the IO thread waiting for the completion port.

      1. main.cpp
        1 kB
        Chuck Rolke

        Activity

        Hide
        Chuck Rolke added a comment -

        Heartbeat timeout test code.

        Show
        Chuck Rolke added a comment - Heartbeat timeout test code.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4383/
        -----------------------------------------------------------

        Review request for qpid, Andrew Stitcher, Ted Ross, Chug Rolke, and Steve Huston.

        Summary
        -------

        The cause of the hang was an outstanding read side completion when the AsynchIO object in charge of the socket was in the queuedClose state.

        The completion handler drains outstanding async requests before closing the socket. Since the cable had been pulled, the async read would never complete until Windows gave up on the socket altogether (some time much later).

        This patch remembers the last aio read and will cancel it if in the queuedClose state before blocking again.

        Aside from the basic description from the Jira, I also removed an unused test for restartRead, which doesn't change the logic of the section, but may indicate an intention that wasn't fully coded or something left over from a previous change.

        This addresses bug QPID-3759.
        https://issues.apache.org/jira/browse/QPID-3759

        Diffs


        http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/sys/windows/AsynchIO.cpp 1301636

        Diff: https://reviews.apache.org/r/4383/diff

        Testing
        -------

        qpid-perftest, qpid-send, qpid-receive, cable pulls, broker pause/resumes

        Thanks,

        Cliff

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4383/ ----------------------------------------------------------- Review request for qpid, Andrew Stitcher, Ted Ross, Chug Rolke, and Steve Huston. Summary ------- The cause of the hang was an outstanding read side completion when the AsynchIO object in charge of the socket was in the queuedClose state. The completion handler drains outstanding async requests before closing the socket. Since the cable had been pulled, the async read would never complete until Windows gave up on the socket altogether (some time much later). This patch remembers the last aio read and will cancel it if in the queuedClose state before blocking again. Aside from the basic description from the Jira, I also removed an unused test for restartRead, which doesn't change the logic of the section, but may indicate an intention that wasn't fully coded or something left over from a previous change. This addresses bug QPID-3759 . https://issues.apache.org/jira/browse/QPID-3759 Diffs http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/sys/windows/AsynchIO.cpp 1301636 Diff: https://reviews.apache.org/r/4383/diff Testing ------- qpid-perftest, qpid-send, qpid-receive, cable pulls, broker pause/resumes Thanks, Cliff
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4383/#review6045
        -----------------------------------------------------------

        Ship it!

        Looks good to me.

        • Steve

        On 2012-03-16 17:32:02, Cliff Jansen wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4383/

        -----------------------------------------------------------

        (Updated 2012-03-16 17:32:02)

        Review request for qpid, Andrew Stitcher, Ted Ross, Chug Rolke, and Steve Huston.

        Summary

        -------

        The cause of the hang was an outstanding read side completion when the AsynchIO object in charge of the socket was in the queuedClose state.

        The completion handler drains outstanding async requests before closing the socket. Since the cable had been pulled, the async read would never complete until Windows gave up on the socket altogether (some time much later).

        This patch remembers the last aio read and will cancel it if in the queuedClose state before blocking again.

        Aside from the basic description from the Jira, I also removed an unused test for restartRead, which doesn't change the logic of the section, but may indicate an intention that wasn't fully coded or something left over from a previous change.

        This addresses bug QPID-3759.

        https://issues.apache.org/jira/browse/QPID-3759

        Diffs

        -----

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/sys/windows/AsynchIO.cpp 1301636

        Diff: https://reviews.apache.org/r/4383/diff

        Testing

        -------

        qpid-perftest, qpid-send, qpid-receive, cable pulls, broker pause/resumes

        Thanks,

        Cliff

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4383/#review6045 ----------------------------------------------------------- Ship it! Looks good to me. Steve On 2012-03-16 17:32:02, Cliff Jansen wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4383/ ----------------------------------------------------------- (Updated 2012-03-16 17:32:02) Review request for qpid, Andrew Stitcher, Ted Ross, Chug Rolke, and Steve Huston. Summary ------- The cause of the hang was an outstanding read side completion when the AsynchIO object in charge of the socket was in the queuedClose state. The completion handler drains outstanding async requests before closing the socket. Since the cable had been pulled, the async read would never complete until Windows gave up on the socket altogether (some time much later). This patch remembers the last aio read and will cancel it if in the queuedClose state before blocking again. Aside from the basic description from the Jira, I also removed an unused test for restartRead, which doesn't change the logic of the section, but may indicate an intention that wasn't fully coded or something left over from a previous change. This addresses bug QPID-3759 . https://issues.apache.org/jira/browse/QPID-3759 Diffs ----- http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/sys/windows/AsynchIO.cpp 1301636 Diff: https://reviews.apache.org/r/4383/diff Testing ------- qpid-perftest, qpid-send, qpid-receive, cable pulls, broker pause/resumes Thanks, Cliff
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4383/#review6048
        -----------------------------------------------------------

        Ship it!

        • Chug

        On 2012-03-16 17:32:02, Cliff Jansen wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4383/

        -----------------------------------------------------------

        (Updated 2012-03-16 17:32:02)

        Review request for qpid, Andrew Stitcher, Ted Ross, Chug Rolke, and Steve Huston.

        Summary

        -------

        The cause of the hang was an outstanding read side completion when the AsynchIO object in charge of the socket was in the queuedClose state.

        The completion handler drains outstanding async requests before closing the socket. Since the cable had been pulled, the async read would never complete until Windows gave up on the socket altogether (some time much later).

        This patch remembers the last aio read and will cancel it if in the queuedClose state before blocking again.

        Aside from the basic description from the Jira, I also removed an unused test for restartRead, which doesn't change the logic of the section, but may indicate an intention that wasn't fully coded or something left over from a previous change.

        This addresses bug QPID-3759.

        https://issues.apache.org/jira/browse/QPID-3759

        Diffs

        -----

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/sys/windows/AsynchIO.cpp 1301636

        Diff: https://reviews.apache.org/r/4383/diff

        Testing

        -------

        qpid-perftest, qpid-send, qpid-receive, cable pulls, broker pause/resumes

        Thanks,

        Cliff

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4383/#review6048 ----------------------------------------------------------- Ship it! Chug On 2012-03-16 17:32:02, Cliff Jansen wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4383/ ----------------------------------------------------------- (Updated 2012-03-16 17:32:02) Review request for qpid, Andrew Stitcher, Ted Ross, Chug Rolke, and Steve Huston. Summary ------- The cause of the hang was an outstanding read side completion when the AsynchIO object in charge of the socket was in the queuedClose state. The completion handler drains outstanding async requests before closing the socket. Since the cable had been pulled, the async read would never complete until Windows gave up on the socket altogether (some time much later). This patch remembers the last aio read and will cancel it if in the queuedClose state before blocking again. Aside from the basic description from the Jira, I also removed an unused test for restartRead, which doesn't change the logic of the section, but may indicate an intention that wasn't fully coded or something left over from a previous change. This addresses bug QPID-3759 . https://issues.apache.org/jira/browse/QPID-3759 Diffs ----- http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/sys/windows/AsynchIO.cpp 1301636 Diff: https://reviews.apache.org/r/4383/diff Testing ------- qpid-perftest, qpid-send, qpid-receive, cable pulls, broker pause/resumes Thanks, Cliff
        Hide
        Chuck Rolke added a comment -

        Verified AsynchIO.cpp r1301636 on Windows Server 2008 R2 Datacenter 64-bit OS, 32-bit app.

        Show
        Chuck Rolke added a comment - Verified AsynchIO.cpp r1301636 on Windows Server 2008 R2 Datacenter 64-bit OS, 32-bit app.
        Hide
        Chuck Rolke added a comment -

        The fix in r1301636 uses function CancelIoEx (Kernel32.lib,.dll) sets a minimum operation version of the client to Windows Vista and of the server to Windows Server 2008. I compiled and tested without realizing that this won't work on XP.

        Show
        Chuck Rolke added a comment - The fix in r1301636 uses function CancelIoEx (Kernel32.lib,.dll) sets a minimum operation version of the client to Windows Vista and of the server to Windows Server 2008. I compiled and tested without realizing that this won't work on XP.
        Hide
        Cliff Jansen added a comment -

        I chose CancelIoEx because it worked most naturally with the existing code. The XP-friendly CancelIo can probably be substituted with minor modification. I will get a new patch fro review asap.

        Show
        Cliff Jansen added a comment - I chose CancelIoEx because it worked most naturally with the existing code. The XP-friendly CancelIo can probably be substituted with minor modification. I will get a new patch fro review asap.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4383/
        -----------------------------------------------------------

        (Updated 2012-03-26 18:26:34.827957)

        Review request for qpid, Andrew Stitcher, Ted Ross, Chug Rolke, and Steve Huston.

        Changes
        -------

        This patch follows the same logic of the previous while avoiding CancelIoEx.

        CancelIo as a substitution for CancelIoEx was considered but has thread restrictions that would have required a major rewrite of the base code.

        I have substituted a much blunter instrument to achieve the completion, namely a full closesocket to unstick the read. It forces all pending overlapped operations to completions, which is the last read in our case.

        Summary
        -------

        The cause of the hang was an outstanding read side completion when the AsynchIO object in charge of the socket was in the queuedClose state.

        The completion handler drains outstanding async requests before closing the socket. Since the cable had been pulled, the async read would never complete until Windows gave up on the socket altogether (some time much later).

        This patch remembers the last aio read and will cancel it if in the queuedClose state before blocking again.

        Aside from the basic description from the Jira, I also removed an unused test for restartRead, which doesn't change the logic of the section, but may indicate an intention that wasn't fully coded or something left over from a previous change.

        This addresses bug QPID-3759.
        https://issues.apache.org/jira/browse/QPID-3759

        Diffs (updated)


        http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/sys/windows/AsynchIO.cpp 1301636

        Diff: https://reviews.apache.org/r/4383/diff

        Testing
        -------

        qpid-perftest, qpid-send, qpid-receive, cable pulls, broker pause/resumes

        Thanks,

        Cliff

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4383/ ----------------------------------------------------------- (Updated 2012-03-26 18:26:34.827957) Review request for qpid, Andrew Stitcher, Ted Ross, Chug Rolke, and Steve Huston. Changes ------- This patch follows the same logic of the previous while avoiding CancelIoEx. CancelIo as a substitution for CancelIoEx was considered but has thread restrictions that would have required a major rewrite of the base code. I have substituted a much blunter instrument to achieve the completion, namely a full closesocket to unstick the read. It forces all pending overlapped operations to completions, which is the last read in our case. Summary ------- The cause of the hang was an outstanding read side completion when the AsynchIO object in charge of the socket was in the queuedClose state. The completion handler drains outstanding async requests before closing the socket. Since the cable had been pulled, the async read would never complete until Windows gave up on the socket altogether (some time much later). This patch remembers the last aio read and will cancel it if in the queuedClose state before blocking again. Aside from the basic description from the Jira, I also removed an unused test for restartRead, which doesn't change the logic of the section, but may indicate an intention that wasn't fully coded or something left over from a previous change. This addresses bug QPID-3759 . https://issues.apache.org/jira/browse/QPID-3759 Diffs (updated) http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/sys/windows/AsynchIO.cpp 1301636 Diff: https://reviews.apache.org/r/4383/diff Testing ------- qpid-perftest, qpid-send, qpid-receive, cable pulls, broker pause/resumes Thanks, Cliff
        Hide
        Steve Huston added a comment -

        With the closesocket instead of CancelIoEx, the nightly quick qpid-perftest crashes trying to close the socket after its impl has been freed.

        Test case:

        • Start broker
        • qpid-perftest --summary --count 100
        Show
        Steve Huston added a comment - With the closesocket instead of CancelIoEx, the nightly quick qpid-perftest crashes trying to close the socket after its impl has been freed. Test case: Start broker qpid-perftest --summary --count 100
        Hide
        Cliff Jansen added a comment -

        Thanks for the heads up. I can reproduce and have a fix in mind.

        Will post asap.

        Show
        Cliff Jansen added a comment - Thanks for the heads up. I can reproduce and have a fix in mind. Will post asap.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4383/
        -----------------------------------------------------------

        (Updated 2012-04-11 02:57:20.281375)

        Review request for qpid, Andrew Stitcher, Ted Ross, Chug Rolke, and Steve Huston.

        Changes
        -------

        The cancelled read usually results in an "aborted" status, but depending on how far the socketclose has progressed at the time the completion is posted, you can get a number of other statuses such as connection reset and several others. This results in a spurious notifyDisconnect() and general mayhem from the deleted Socket.

        Since closesocket() is a relatively long operation and the cancel operation occurred outside the completionQueue loop, the dislodged read completion could be processed in a separate thread resulting in a concurrent Socket::close() which, on occasion, yielded an exception. This was fixed by moving the cancel inside the completionQueue loop so that the resulting completion would be serialized after the cancel.

        So round three involves:

        1. just using queuedClose to indicate a drained read
        2. moving the socket.close() to serialize the read completion
        3. adding a queuedDelete check before using a non-existent socket

        Presumably #2 would never have occurred with CancelIoEx. But it is probable that #1 would have been lurking, just occurring very rarely (depending on whether the other side closed its connection at just the right/wrong time). #3 can be attributed solely to my paranoia.

        Summary
        -------

        The cause of the hang was an outstanding read side completion when the AsynchIO object in charge of the socket was in the queuedClose state.

        The completion handler drains outstanding async requests before closing the socket. Since the cable had been pulled, the async read would never complete until Windows gave up on the socket altogether (some time much later).

        This patch remembers the last aio read and will cancel it if in the queuedClose state before blocking again.

        Aside from the basic description from the Jira, I also removed an unused test for restartRead, which doesn't change the logic of the section, but may indicate an intention that wasn't fully coded or something left over from a previous change.

        This addresses bug QPID-3759.
        https://issues.apache.org/jira/browse/QPID-3759

        Diffs (updated)


        http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/sys/windows/AsynchIO.cpp 1301636

        Diff: https://reviews.apache.org/r/4383/diff

        Testing
        -------

        qpid-perftest, qpid-send, qpid-receive, cable pulls, broker pause/resumes

        Thanks,

        Cliff

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4383/ ----------------------------------------------------------- (Updated 2012-04-11 02:57:20.281375) Review request for qpid, Andrew Stitcher, Ted Ross, Chug Rolke, and Steve Huston. Changes ------- The cancelled read usually results in an "aborted" status, but depending on how far the socketclose has progressed at the time the completion is posted, you can get a number of other statuses such as connection reset and several others. This results in a spurious notifyDisconnect() and general mayhem from the deleted Socket. Since closesocket() is a relatively long operation and the cancel operation occurred outside the completionQueue loop, the dislodged read completion could be processed in a separate thread resulting in a concurrent Socket::close() which, on occasion, yielded an exception. This was fixed by moving the cancel inside the completionQueue loop so that the resulting completion would be serialized after the cancel. So round three involves: 1. just using queuedClose to indicate a drained read 2. moving the socket.close() to serialize the read completion 3. adding a queuedDelete check before using a non-existent socket Presumably #2 would never have occurred with CancelIoEx. But it is probable that #1 would have been lurking, just occurring very rarely (depending on whether the other side closed its connection at just the right/wrong time). #3 can be attributed solely to my paranoia. Summary ------- The cause of the hang was an outstanding read side completion when the AsynchIO object in charge of the socket was in the queuedClose state. The completion handler drains outstanding async requests before closing the socket. Since the cable had been pulled, the async read would never complete until Windows gave up on the socket altogether (some time much later). This patch remembers the last aio read and will cancel it if in the queuedClose state before blocking again. Aside from the basic description from the Jira, I also removed an unused test for restartRead, which doesn't change the logic of the section, but may indicate an intention that wasn't fully coded or something left over from a previous change. This addresses bug QPID-3759 . https://issues.apache.org/jira/browse/QPID-3759 Diffs (updated) http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/sys/windows/AsynchIO.cpp 1301636 Diff: https://reviews.apache.org/r/4383/diff Testing ------- qpid-perftest, qpid-send, qpid-receive, cable pulls, broker pause/resumes Thanks, Cliff
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4383/#review6859
        -----------------------------------------------------------

        Ship it!

        • Chug

        On 2012-04-11 02:57:20, Cliff Jansen wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4383/

        -----------------------------------------------------------

        (Updated 2012-04-11 02:57:20)

        Review request for qpid, Andrew Stitcher, Ted Ross, Chug Rolke, and Steve Huston.

        Summary

        -------

        The cause of the hang was an outstanding read side completion when the AsynchIO object in charge of the socket was in the queuedClose state.

        The completion handler drains outstanding async requests before closing the socket. Since the cable had been pulled, the async read would never complete until Windows gave up on the socket altogether (some time much later).

        This patch remembers the last aio read and will cancel it if in the queuedClose state before blocking again.

        Aside from the basic description from the Jira, I also removed an unused test for restartRead, which doesn't change the logic of the section, but may indicate an intention that wasn't fully coded or something left over from a previous change.

        This addresses bug QPID-3759.

        https://issues.apache.org/jira/browse/QPID-3759

        Diffs

        -----

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/sys/windows/AsynchIO.cpp 1301636

        Diff: https://reviews.apache.org/r/4383/diff

        Testing

        -------

        qpid-perftest, qpid-send, qpid-receive, cable pulls, broker pause/resumes

        Thanks,

        Cliff

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4383/#review6859 ----------------------------------------------------------- Ship it! Chug On 2012-04-11 02:57:20, Cliff Jansen wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4383/ ----------------------------------------------------------- (Updated 2012-04-11 02:57:20) Review request for qpid, Andrew Stitcher, Ted Ross, Chug Rolke, and Steve Huston. Summary ------- The cause of the hang was an outstanding read side completion when the AsynchIO object in charge of the socket was in the queuedClose state. The completion handler drains outstanding async requests before closing the socket. Since the cable had been pulled, the async read would never complete until Windows gave up on the socket altogether (some time much later). This patch remembers the last aio read and will cancel it if in the queuedClose state before blocking again. Aside from the basic description from the Jira, I also removed an unused test for restartRead, which doesn't change the logic of the section, but may indicate an intention that wasn't fully coded or something left over from a previous change. This addresses bug QPID-3759 . https://issues.apache.org/jira/browse/QPID-3759 Diffs ----- http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/sys/windows/AsynchIO.cpp 1301636 Diff: https://reviews.apache.org/r/4383/diff Testing ------- qpid-perftest, qpid-send, qpid-receive, cable pulls, broker pause/resumes Thanks, Cliff
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4383/
        -----------------------------------------------------------

        (Updated 2012-04-19 06:52:43.361572)

        Review request for qpid, Andrew Stitcher, Ted Ross, Chug Rolke, and Steve Huston.

        Changes
        -------

        Load tests over a period of time reveal a threading bug when closing a connection.

        The testing for opsInprogress == 0 and the states of queuedDelete and queuedClose occurs outside the lock. If an IO thread suspends right after releasing the lock (opsInProgress == 1) and resumes some time later, when another IO thread has decremented opsInProgress to zero, both threads will conclude that they are the last IO completion. This results variously in double deletes of the underlying socket or the AsynchIO object itself.

        This patch moves the test inside the lock.

        It also uses the same lock to protect the setting of either queuedDelete or queuedClose and the handoff (if any) to the IO thread. This has the effect of adding two additional locks over the life of the connection, but should have no effect on throughput or latency.

        Summary
        -------

        The cause of the hang was an outstanding read side completion when the AsynchIO object in charge of the socket was in the queuedClose state.

        The completion handler drains outstanding async requests before closing the socket. Since the cable had been pulled, the async read would never complete until Windows gave up on the socket altogether (some time much later).

        This patch remembers the last aio read and will cancel it if in the queuedClose state before blocking again.

        Aside from the basic description from the Jira, I also removed an unused test for restartRead, which doesn't change the logic of the section, but may indicate an intention that wasn't fully coded or something left over from a previous change.

        This addresses bug QPID-3759.
        https://issues.apache.org/jira/browse/QPID-3759

        Diffs (updated)


        http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/sys/windows/AsynchIO.cpp 1327776

        Diff: https://reviews.apache.org/r/4383/diff

        Testing
        -------

        qpid-perftest, qpid-send, qpid-receive, cable pulls, broker pause/resumes

        Thanks,

        Cliff

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4383/ ----------------------------------------------------------- (Updated 2012-04-19 06:52:43.361572) Review request for qpid, Andrew Stitcher, Ted Ross, Chug Rolke, and Steve Huston. Changes ------- Load tests over a period of time reveal a threading bug when closing a connection. The testing for opsInprogress == 0 and the states of queuedDelete and queuedClose occurs outside the lock. If an IO thread suspends right after releasing the lock (opsInProgress == 1) and resumes some time later, when another IO thread has decremented opsInProgress to zero, both threads will conclude that they are the last IO completion. This results variously in double deletes of the underlying socket or the AsynchIO object itself. This patch moves the test inside the lock. It also uses the same lock to protect the setting of either queuedDelete or queuedClose and the handoff (if any) to the IO thread. This has the effect of adding two additional locks over the life of the connection, but should have no effect on throughput or latency. Summary ------- The cause of the hang was an outstanding read side completion when the AsynchIO object in charge of the socket was in the queuedClose state. The completion handler drains outstanding async requests before closing the socket. Since the cable had been pulled, the async read would never complete until Windows gave up on the socket altogether (some time much later). This patch remembers the last aio read and will cancel it if in the queuedClose state before blocking again. Aside from the basic description from the Jira, I also removed an unused test for restartRead, which doesn't change the logic of the section, but may indicate an intention that wasn't fully coded or something left over from a previous change. This addresses bug QPID-3759 . https://issues.apache.org/jira/browse/QPID-3759 Diffs (updated) http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/sys/windows/AsynchIO.cpp 1327776 Diff: https://reviews.apache.org/r/4383/diff Testing ------- qpid-perftest, qpid-send, qpid-receive, cable pulls, broker pause/resumes Thanks, Cliff
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4383/#review7069
        -----------------------------------------------------------

        Ship it!

        I spun up VS2008 and VS2010, x86 and x64, debug and release versions of the C++ and .NET binding tools and ran 10's of thousands of these executables against each other with no problem. Previous versions of tests built with patches on this review (on 64-bit Server 2008 R2 Datacenter) usually showed some executable failures before this many executions.

        • Chug

        On 2012-04-19 06:52:43, Cliff Jansen wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4383/

        -----------------------------------------------------------

        (Updated 2012-04-19 06:52:43)

        Review request for qpid, Andrew Stitcher, Ted Ross, Chug Rolke, and Steve Huston.

        Summary

        -------

        The cause of the hang was an outstanding read side completion when the AsynchIO object in charge of the socket was in the queuedClose state.

        The completion handler drains outstanding async requests before closing the socket. Since the cable had been pulled, the async read would never complete until Windows gave up on the socket altogether (some time much later).

        This patch remembers the last aio read and will cancel it if in the queuedClose state before blocking again.

        Aside from the basic description from the Jira, I also removed an unused test for restartRead, which doesn't change the logic of the section, but may indicate an intention that wasn't fully coded or something left over from a previous change.

        This addresses bug QPID-3759.

        https://issues.apache.org/jira/browse/QPID-3759

        Diffs

        -----

        http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/sys/windows/AsynchIO.cpp 1327776

        Diff: https://reviews.apache.org/r/4383/diff

        Testing

        -------

        qpid-perftest, qpid-send, qpid-receive, cable pulls, broker pause/resumes

        Thanks,

        Cliff

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4383/#review7069 ----------------------------------------------------------- Ship it! I spun up VS2008 and VS2010, x86 and x64, debug and release versions of the C++ and .NET binding tools and ran 10's of thousands of these executables against each other with no problem. Previous versions of tests built with patches on this review (on 64-bit Server 2008 R2 Datacenter) usually showed some executable failures before this many executions. Chug On 2012-04-19 06:52:43, Cliff Jansen wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4383/ ----------------------------------------------------------- (Updated 2012-04-19 06:52:43) Review request for qpid, Andrew Stitcher, Ted Ross, Chug Rolke, and Steve Huston. Summary ------- The cause of the hang was an outstanding read side completion when the AsynchIO object in charge of the socket was in the queuedClose state. The completion handler drains outstanding async requests before closing the socket. Since the cable had been pulled, the async read would never complete until Windows gave up on the socket altogether (some time much later). This patch remembers the last aio read and will cancel it if in the queuedClose state before blocking again. Aside from the basic description from the Jira, I also removed an unused test for restartRead, which doesn't change the logic of the section, but may indicate an intention that wasn't fully coded or something left over from a previous change. This addresses bug QPID-3759 . https://issues.apache.org/jira/browse/QPID-3759 Diffs ----- http://svn.apache.org/repos/asf/qpid/trunk/qpid/cpp/src/qpid/sys/windows/AsynchIO.cpp 1327776 Diff: https://reviews.apache.org/r/4383/diff Testing ------- qpid-perftest, qpid-send, qpid-receive, cable pulls, broker pause/resumes Thanks, Cliff
        Hide
        Robbie Gemmell added a comment -

        There are 4 commits for this from 5+ months ago so im going to assume its done, resolving.

        Show
        Robbie Gemmell added a comment - There are 4 commits for this from 5+ months ago so im going to assume its done, resolving.

          People

          • Assignee:
            Cliff Jansen
            Reporter:
            Chuck Rolke
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development