[PATCH v2 2/2] ntdll: sock_recv keeps message ordered

[Zebediah Figura]

On 12/7/21 18:20, Dongwan Kim wrote:
> Thanks to review my patch agian.
> 
> I need this kind of patch to run KakaoTalk which is the dominant messenger application in Korea.
> All communication between client and server is done this way.
> So, there is a problem in Login and  Sending files currently.
> 
> 
> I've analyzed the appplication and made prototype server/client programs to test the problem.
> The client sends messages, which are part of a file, to the server.
> The server assembles the messages and write them to a file.
> And compare the result file with the original one. If two files are different, there is a problem in message ordering.
> 
> Actually, the other patch is originated from this test.
> 
> I'll try embed the test into winetest, but I dont't know what file is chosen to test.
> Could you give me an advice?
> As you know, this test is not deterministic for a small file because there are few chances to cut in line.
> I use 96MB file to test and it always guarantees the test result.

You can synthesize data (note that you don't really need a file anyway). 
I might suggest something like this:

int buffer[200];

for (i = 0; i < 100; ++i)
{
     for (j = 0; j < sizeof(buffer); ++j)
         buffer[j] = (i * sizeof(buffer)) + j;
     send(client, buffer, sizeof(buffer), 0);
}

and then, when receiving:

total = 0;

while (total < 100 * sizeof(buffer))
{
     size = recv(server, buffer, sizeof(buffer), 0);
     for (i = 0; i < size; ++i)
         ok(buffer[i] == total + i);
     total += size;
}

Tweak as necessary (bigger buffers, or whatever) so that it consistently 
reproduces the problem on current Wine.

> Also, to convince you quickly that there is a problem, could I send you the code of the test programs?

As long as you've tested, I can trust your word ;-)

But of course a test in Wine's tree would be great to have.

> Last question, sorry. What is in-tree/out-of-tree test?

"In-tree" means it'd be added to Wine's unit test suite, which is 
located in the git tree, under dlls/*/tests/. In this case you'd want to 
put a new test in dlls/ws2_32/tests/sock.c.

So, with my fears about the necessity of this feature confirmed, I think 
the right way to implement it is actually to unconditionally get rid of 
the initial try_recv call. Trying to do it conditionally is pretty much 
impossible without races.

This will, however, introduce an extra server call in some cases. 
Specifically, if there is already data available (and we are the first 
in line) we currently make two server calls (one to report 
STATUS_SUCCESS to the server, and one to select on the async handle). 
With this change we'll make three. (Note that it looks like two, because 
we're still doing a call to NtWaitForSingleObject, but being interrupted 
by a kernel APC means we restart the select call.)

In order to solve this, my suggestion would be to let the recv_socket 
request return an initial status of STATUS_ALERTED. This would be a 
signal to the Unix side to try recv() immediately, instead of calling 
select and letting the server interrupt us. We would need a new server 
request to get at async_set_result(), since the "usual" way of doing 
that requires an APC handle. As an optimization this could (and 
therefore probably should) be done independently, in a separate patch, 
and as such should probably be ordered before the removal of the initial 
try_recv() call, to avoid a temporary performance regression.

On the other hand, the fact that most server socket calls take at least 
two requests already makes it a bit unclear whether adding a third will 
cause a performance problem. Socket I/O is, after all, generally 
asynchronous, and not exactly as low-latency as, say, NT synchronization 
primitives. Although I'm very hesitant to make existing code worse, and 
although the above solution isn't exactly complex, I also kind of wonder 
if it's worthwhile...