[PATCH] server: Add new pipe servers to the end of the servers list.

[Huw Davies]

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 
---
 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 );

[Jacek Caban]

Signed-off-by: Jacek Caban

[Jacek Caban]

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

[Marvin]

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

[Huw Davies]

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