[Bug 52401] Improper synchronization in sock_recv/sock_send leads to arbitrary reordering of completion of I/O requests

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=52401

--- Comment #4 from Jinoh Kang <jinoh.kang.kr at gmail.com> ---
(In reply to Paul Gofman from comment #3)
> I’d expect solution 1 to have a very unfortunate network performance impact,
> this will make even fully synchronous socket io always take a server async

We're already doing that.  Notice that 'recv_socket' is still called even when
try_recv() returns STATUS_SUCCESS; therefore, a server async is still
allocated.
This behaviour is complete with sock_recv() calling wait_async() on the
returned
wait_handle, which async_handoff() leaves open for synchronously satisfied
requests so that completion ports can be notified and APCs called.

> and possibly SIGUSR roundtrip.

This is the real problem.  We can solve this by coalescing the 'recv_socket'
and 'select' replies into one packet.  Basically the algorithm would go like
this:

1. The client issues a 'recv_socket' call.
2. The server determines if read_q is empty *and* the request can be served
   immediately.
3. If this is the case, the server does the following:
   1. The server marks the current thread as being in system APC wait.
   2. The server bypasses sock->read_q and calls async_terminate() directly.
      (Alternatively, the server first puts the async in read_q and later
       calls async_wake_up( &sock->read_q ).)
   3. The server dequeues the next system APC (likely APC_ASYNC_IO from 3.2)
      from the queue.
   4. The server inserts the APC into the 'recv_socket' reply.

Ideally we could make it so that every Wineserver call has a flag that
indicates
whether it could process the original request *and* retrieve the next system
APC
from the queue in one go.  This will eliminate the extra SIGUSR1 round trip in
other cases as well.

> Note also that the sockets created with
> socket() are capable of async io by default, so checking for socket flags
> won’t change much in the majority of cases. This probably needs something
> neater. I don’t know, maybe some sort of io request versioning can work?

I'm afraid that this problem can be reproduced even with fully synchronous
socket I/O operations.  Just do the second WSARecv (or recv) call in step 4 in 
another thread.

-- 
Do not reply to this email, post in Bugzilla using the
above URL to reply.
You are receiving this mail because:
You are watching all bug changes.