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