Description

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).

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

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.

jiraposter@reviews.apache.org
added a comment - 16/Mar/12 17:33
-----------------------------------------------------------
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

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.

jiraposter@reviews.apache.org
added a comment - 16/Mar/12 18:07
-----------------------------------------------------------
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

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.

jiraposter@reviews.apache.org
added a comment - 16/Mar/12 18:31
-----------------------------------------------------------
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

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.

Chuck Rolke
added a comment - 22/Mar/12 16:10 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.

Cliff Jansen
added a comment - 22/Mar/12 17:09 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.

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

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.

jiraposter@reviews.apache.org
added a comment - 26/Mar/12 18:28
-----------------------------------------------------------
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

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

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.

jiraposter@reviews.apache.org
added a comment - 11/Apr/12 02:58
-----------------------------------------------------------
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

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.

jiraposter@reviews.apache.org
added a comment - 11/Apr/12 19:35
-----------------------------------------------------------
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

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

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.

jiraposter@reviews.apache.org
added a comment - 19/Apr/12 07:07
-----------------------------------------------------------
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

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.

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.

jiraposter@reviews.apache.org
added a comment - 20/Apr/12 13:50
-----------------------------------------------------------
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