Boost C++ Libraries: Ticket #8967: Spurious zero bytes written and eof notifications highlight ASIO IOCP issues

attachment set

simoncperkins@… — Mon, 05 Aug 2013 16:14:30 GMT

attachment → patchfile.zip

Had to zip the patch file because the comments contain links to MSDN docs

Sat, 10 Aug 2013 07:06:47 GMT

Hi,

I found and fixed the same problem: https://svn.boost.org/trac/boost/ticket/8933

I disagree with your patch tho as I think it's dangerous removing how the on_pending works. It's there to delay the calling of the complete handler until after the returning of the winsock functions as they are not reentrant for a single socket. Removing the it completely would result in existing asio programs not working correctly in all situations.

The ASIO documentation says the the complete handler is not called until after the call to the sockets function has returned. This means that it's safe to call the next read/write cycle again. Taking away how the on_pending works will mean that it's possible that you can call back into the read/write functions before you are allowed to, which can caused undefined behaviour.

Quote from MSDN: WSARecv should not be called on the same socket simultaneously from different threads, because it can result in an unpredictable buffer order.

Check out my patch, it's much simpler (basically it reverts the code back to how it used to work), as the actual bug is that on_pending should be calling PostQueuedCompletionStatus with the 'overlapped_contains_result' value so that the io completion routine knows that it's a delayed result rather than a real result. Because it got changed from 'overlapped_contains_result' to zero, the io completion routine thinks it's a result from a winsock type function rather than a delayed result where the result is stored in the overlapped structure.

simoncperkins@… — Sat, 10 Aug 2013 11:37:19 GMT

Thanks for your comment. I don't believe that handling the notification directly without on_pending is a problem for the following reasons:

1) The existing I/O operation has already completed (even though the API may not have returned) if another thread picks up the notification using GetQueueCompletionStatus().

2) None of the multi-threaded samples I looked at on MSDN worry about calling WSARecv/WSASend in a notification handler even though the originating API may not have returned.

3) The MSDN docs for WSARecv explicitly state that this is acceptable for completion handlers:

* "The WSARecv function using overlapped I/O can be called from within the completion routine of a previous WSARecv, WSARecvFrom, WSASend or WSASendTo function. For a given socket, I/O completion routines will not be nested. This permits time-sensitive data transmissions to occur entirely within a preemptive context." *

The IOCP / GetQueueCompletionStatus mechanism is just a way of running the completion handler on a pool thread.

It's not clear to me how the on_pending function should work. When you call PostQueuedCompletionStatus it doesn't even care if you supply an overlapped structure (google for 'The parameters to PostQueuedCompletionStatus are not interpreted' ). It can be read by GetQueueedCompletionStatus independently of whether the I/O has completed. You would have to use a call to GetOverlappedResult to wait for the results.

It looks to me like the code around on_pending and win_iocp_io_service::do_one has been written assuming that the notification from on_pending will be delivered when the I/O is complete. That's not the case - it just delivers the parameter values from PostQueuedCompletionStatus when the next thread returns from GetQueueedCompletionStatus. It does not look at the I/O operation associated with the overlapped structure.

Although, my patch contains more changes I think it has the following benefits:

1) It results in less code in the ASIO IOCP implementation

2) During normal operations PostQueuedCompletionStatus is only called when an I/O immediately fails. I believe this could have a performance benefit. Currently, it is called for every non-failing I/O operation effectively doubling the number of notifications that have to be handled by the service threads.

3) The code is closer to the structure of the IOCP samples that Microsoft provides

4) I believe the on_pending code has a problem because it's notification may be dequeued before the I/O has completed

Sat, 10 Aug 2013 21:28:28 GMT

Looking at the asio code up util 1.53.0, the on_pending is used to delay the result of a early completion routine result until after the OS call (WSARecv/WSASend/...) has been completed.

It works because the on_pending only calls PostQueuedCompletionStatus if op ready_ was already 1. i.e. the completion routine has already fired. It doesn't post a PostQueuedCompletionStatus if the completion routine hasn't fired, it just sets the ready_ to 1 which allows the the callers completion handler to be fired in the completion routine. So I don't think there is a problem with the on_pending code.

So the change to 1.54.0 from 'overlapped_contains_result' to using '0' is what is causing all the problems.

I can also see you may be correct is that it's not actually required in the case of WSARecv/WSASend/WSA* winsock functions to use the op ready_/overlapped_contains_result. It's used in other places so you may not be able to get rid of the overlapped_contains_result and storing the results in the overlapped structure tho.

Also you would have to check all the other places that use the on_pending to see if it's completely ok to remove the ready flag ok as well. I see it's used with ReadFile/WriteFile/AcceptEx

status changed; resolution set

chris_kohlhoff — Tue, 01 Oct 2013 11:06:12 GMT

status new → closed
resolution → invalid

The OVERLAPPED object must remain valid until *both* the following conditions are true:

1) The initiating function (e.g. WSARecv) has returned.

2) The completion packet has been dequeued via GetQueuedCompletionStatus.

Freeing the memory immediately after (2) can result in an access violation if (1) has not yet happened. The on_pending() function and the ready_ flag exist to cover the case where (2) happens before (1).

Boost 1.54 had a regression that prevented on_pending() from working correctly. This has now been fixed. See #8933.

simoncperkins@… — Tue, 01 Oct 2013 11:18:18 GMT

Ok, thanks for fixing the problem but as stated above I disagree with your points.

I believe you can safely free the overlapped structure in 2) if the IO in 1) has completed. That's how IO completion ports are intended to work.

The current implementation is sub-optimal because it makes unnecessary calls to PostQueuedCompletionStatus .

Smueller@… — Wed, 26 Feb 2014 05:36:51 GMT

Chris, have you seen situations where dequeuing from an iocompletionport has occurred before the wsarecv has returned? If so? Have you seen access violations from this occurence?

chris_kohlhoff — Mon, 05 May 2014 08:53:00 GMT

Replying to Smueller@…:

Chris, have you seen situations where dequeuing from an iocompletionport has occurred before the wsarecv has returned? If so? Have you seen access violations from this occurence?

Yes and yes.

simoncperkins@… — Tue, 06 May 2014 08:47:03 GMT

Chris,

When you had the access violations were you using non-null lpNumberOfBytesRecvd / lpNumberOfBytesSent parameter values for WSARecv/WSASend like the current Boost ASIO implementation?

The MSDN docs for both functions clearly state that these parameters should be NULL for overlapped I/O: "Use NULL for this parameter if the lpOverlapped parameter is not NULL to avoid potentially erroneous results. This parameter can be NULL only if the lpOverlapped parameter is not NULL."

Perhaps, the APIs attempt to access the overlapped structure after completion using GetOverlappedResult to extract the bytes read/written value and that's what causes the access violation.

We've been running with my diffed code on Windows 7 and 8 for nine months now both in release and debug with no access violations. However, the code does use NULL for the lpNumberOfBytesRecvd / lpNumberOfBytesSent parameters.

chris_kohlhoff — Tue, 06 May 2014 09:00:06 GMT

The MSDN docs are clearly stating the wrong thing :) That parameter must be non-null or you will get other crashes on some versions of Windows, and you can see that in some of the comments at the bottom of the WSARecv page. It may no longer be a problem with Windows 7 or later, but it was definitely the case on older versions of Windows.

anonymous — Tue, 06 May 2014 22:42:32 GMT

Hi Chris, the comment on WSARecv in MSDN doesn't seem relevant, as the commenter was trying to look at first chance exceptions thrown, and handled, by the kernel. Can you give us more details as to what the versions of Windows were where you saw the problem?

chris_kohlhoff — Wed, 07 May 2014 00:07:29 GMT

The comment:

lpNumberOfBytesRecvd

...

Use NULL for this parameter if the lpOverlapped parameter is not NULL to avoid potentially erroneous results

is BAD advice.

If call WSARecv and pass NULL as lpNumberOfBytesRecvd and not-NULL lpOverlapped, then your application will generate a bunch of these:

Access violation - code c0000005 (first chance)

describes the problem exactly. I encountered it on Windows XP, or possibly 2000, but I no longer have the system details. Furthermore, the MSDN Library shipped with Visual Studio 2008 did not include the "advice" for lpNumberOfBytesRecvd. What it does say is:

If an overlapped operation completes immediately, WSARecv returns a value of zero and the lpNumberOfBytesRecvd parameter is updated with the number of bytes received ...

This text still appears in the latest MSDN, for what it's worth.