[PATCH] server: Add new pipe servers to the end of the servers list.
This ensures instances created first will be connected to first. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=41670 Signed-off-by: Huw Davies <huw(a)codeweavers.com> --- dlls/kernel32/tests/pipe.c | 38 ++++++++++++++++++++++++++++++++++++++ server/named_pipe.c | 2 +- 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/dlls/kernel32/tests/pipe.c b/dlls/kernel32/tests/pipe.c index 5bf27fac90..5dc6ace731 100644 --- a/dlls/kernel32/tests/pipe.c +++ b/dlls/kernel32/tests/pipe.c @@ -3671,6 +3671,43 @@ static void test_namedpipe_session_id(void) CloseHandle(server); } +static void test_multiple_instances(void) +{ + HANDLE server[2], client; + int i; + BOOL ret; + OVERLAPPED ov; + + for (i = 0; i < ARRAY_SIZE(server); i++) + { + server[i] = CreateNamedPipeA(PIPENAME, PIPE_ACCESS_DUPLEX | FILE_FLAG_OVERLAPPED, + PIPE_READMODE_BYTE | PIPE_WAIT, 2, 1024, 1024, + NMPWAIT_USE_DEFAULT_WAIT, NULL); + ok(server[i] != INVALID_HANDLE_VALUE, "got invalid handle\n"); + } + + client = CreateFileA(PIPENAME, GENERIC_READ | GENERIC_WRITE, 0, NULL, OPEN_EXISTING, 0, 0); + ok(client != INVALID_HANDLE_VALUE, "got invalid handle\n"); + + /* Show that this has connected to server[0] not server[1] */ + + memset(&ov, 0, sizeof(ov)); + ret = ConnectNamedPipe(server[1], &ov); + ok(ret == FALSE, "got %d\n", ret); + ok(GetLastError() == ERROR_IO_PENDING, "got %d\n", GetLastError()); + + memset(&ov, 0, sizeof(ov)); + ret = ConnectNamedPipe(server[0], &ov); + ok(ret == FALSE, "got %d\n", ret); + ok(GetLastError() == ERROR_PIPE_CONNECTED, "got %d\n", GetLastError()); + + DisconnectNamedPipe(server[1]); + DisconnectNamedPipe(server[0]); + CloseHandle(client); + CloseHandle(server[1]); + CloseHandle(server[0]); +} + START_TEST(pipe) { char **argv; @@ -3736,4 +3773,5 @@ START_TEST(pipe) test_TransactNamedPipe(); test_namedpipe_process_id(); test_namedpipe_session_id(); + test_multiple_instances(); } diff --git a/server/named_pipe.c b/server/named_pipe.c index 101ff632b9..9e3303259f 100644 --- a/server/named_pipe.c +++ b/server/named_pipe.c @@ -1162,7 +1162,7 @@ static struct pipe_server *create_pipe_server( struct named_pipe *pipe, unsigned server->pipe_end.server_pid = get_process_id( current->process ); init_async_queue( &server->listen_q ); - list_add_head( &pipe->servers, &server->entry ); + list_add_tail( &pipe->servers, &server->entry ); if (!(server->pipe_end.fd = alloc_pseudo_fd( &pipe_server_fd_ops, &server->pipe_end.obj, options ))) { release_object( server ); -- 2.18.0
Hi Huw, On 2/26/19 12:06 PM, Huw Davies wrote:
This ensures instances created first will be connected to first.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=41670 Signed-off-by: Huw Davies <huw(a)codeweavers.com> --- dlls/kernel32/tests/pipe.c | 38 ++++++++++++++++++++++++++++++++++++++ server/named_pipe.c | 2 +- 2 files changed, 39 insertions(+), 1 deletion(-)
While the patch looks good and has my sign-off, note that it's quite incomplete. For example, the order of used servers takes into account order of ConnectNamedPipe() calls. I extended your tests to show that it's quite incorrect. Also current code will prefer servers created earlier even if they just entered listening state while others are waiting longer. Could you please try the attached follow-up patch and see if the app is fine with it? Thanks, Jacek
Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=48287 Your paranoid android. === build (build log) === error: patch failed: dlls/kernel32/tests/pipe.c:3673 error: patch failed: server/named_pipe.c:1162 Task: Patch failed to apply === debian9 (build log) === error: patch failed: dlls/kernel32/tests/pipe.c:3673 error: patch failed: server/named_pipe.c:1162 Task: Patch failed to apply === debian9 (build log) === error: patch failed: dlls/kernel32/tests/pipe.c:3673 error: patch failed: server/named_pipe.c:1162 Task: Patch failed to apply
On Tue, Feb 26, 2019 at 02:56:59PM +0100, Jacek Caban wrote:
On 2/26/19 12:06 PM, Huw Davies wrote:
This ensures instances created first will be connected to first.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=41670 Signed-off-by: Huw Davies <huw(a)codeweavers.com> --- dlls/kernel32/tests/pipe.c | 38 ++++++++++++++++++++++++++++++++++++++ server/named_pipe.c | 2 +- 2 files changed, 39 insertions(+), 1 deletion(-)
While the patch looks good and has my sign-off, note that it's quite incomplete. For example, the order of used servers takes into account order of ConnectNamedPipe() calls. I extended your tests to show that it's quite incorrect. Also current code will prefer servers created earlier even if they just entered listening state while others are waiting longer.
Could you please try the attached follow-up patch and see if the app is fine with it?
Hi Jacek, Thanks for digging further. Yes, the app is still fine with this patch. One thing, in the call to CreateNamedPipeA() in the tests, let's pass ARRAY_SIZE(server) as the max_instances param - my fault. Thanks, Huw.
participants (3)
-
Huw Davies -
Jacek Caban -
Marvin