Re: [rpcrt4] Properly created named pipes
Robert Shearman wrote: > Thomas Weidenmueller wrote: > If what you say is true about the PIPE_NOWAIT flag affecting this then > it should be easy to fix. The PIPE_NOWAIT flag is documented in the PSDK. In fact, named pipes are handled slightly differently in ReadFile/WriteFile. Even if the named pipe was created with FILE_FLAG_OVERLAPPED the connect, read write operations are still performed synchronously, unless the pipe was created with PIPE_NOWAIT. This is because named pipes are handled differently to other devices and it's up to the npfs driver to decide whether to perform the operation synchronously or not (by checking for PIPE_NOWAIT, which is internally mapped to FILE_PIPE_COMPLETE_OPERATION). So all in all, my patch in fact isn't quite correct because the PIPE_NOWAIT flag would be necessary. > No. I'm pretty sure that the code works correctly and doesn't leak > threads in Wine. If you can get a backtrace of the threads that > shouldn't be around then I might be able to determine what is going wrong. Ok, then I guess it's the pipe implementation in ROS that causes this handle leak. - Thomas
[rpcrt4] Properly created named pipes
Created the named pipes with the FILE_FLAG_OVERLAPPED flag, otherwise
the call to ConnectNamedPipe() will block the server thread if no
connection can be established, which causes the rpc server to dead-lock
during startup.
- Thomas
Index: dlls/rpcrt4/rpc_binding.c
===
RCS file: /home/wine/wine/dlls/rpcrt4/rpc_binding.c,v
retrieving revision 1.39
diff -u -r1.39 rpc_binding.c
--- dlls/rpcrt4/rpc_binding.c 6 Sep 2005 10:26:14 - 1.39
+++ dlls/rpcrt4/rpc_binding.c 16 Jan 2006 23:00:15 -
@@ -142,7 +142,7 @@
pname = HeapAlloc(GetProcessHeap(), 0, strlen(prefix) +
strlen(Connection->Endpoint) + 1);
strcat(strcpy(pname, prefix), Connection->Endpoint);
TRACE("listening on %s\n", pname);
-Connection->conn = CreateNamedPipeA(pname, PIPE_ACCESS_DUPLEX,
+Connection->conn = CreateNamedPipeA(pname, PIPE_ACCESS_DUPLEX |
FILE_FLAG_OVERLAPPED,
PIPE_TYPE_MESSAGE |
PIPE_READMODE_MESSAGE, PIPE_UNLIMITED_INSTANCES,
RPC_MAX_PACKET_SIZE,
RPC_MAX_PACKET_SIZE, 5000, NULL);
HeapFree(GetProcessHeap(), 0, pname);
@@ -166,7 +166,7 @@
pname = HeapAlloc(GetProcessHeap(), 0, strlen(prefix) +
strlen(Connection->Endpoint) + 1);
strcat(strcpy(pname, prefix), Connection->Endpoint);
TRACE("listening on %s\n", pname);
-Connection->conn = CreateNamedPipeA(pname, PIPE_ACCESS_DUPLEX,
+Connection->conn = CreateNamedPipeA(pname, PIPE_ACCESS_DUPLEX |
FILE_FLAG_OVERLAPPED,
PIPE_TYPE_MESSAGE |
PIPE_READMODE_MESSAGE | PIPE_WAIT, PIPE_UNLIMITED_INSTANCES,
RPC_MAX_PACKET_SIZE,
RPC_MAX_PACKET_SIZE, 5000, NULL);
HeapFree(GetProcessHeap(), 0, pname);
Re: [rpcrt4] Properly created named pipes
Thomas Weidenmueller wrote: Robert Shearman wrote: I have a patch that converts rpcrt4 over to using overlapped I/O, but I didn't submit it because the performance on Wine is horrible. When using overlapped I/O we have to perform several more server calls than when using non-overlapped I/O. Also, I think that this patch is incorrect because if you want to make the pipe overlapped then you have to fix up all of the ReadFile calls to take an OVERLAPPED structure. That's true, the ReadFile calls would also have to use the OVERLAPPED structure, but that would only work if the pipes were created with the PIPE_NOWAIT flag. Otherwise ReadFile and WriteFile will (or better should) be still synchronous regardless of the FILE_FLAG_OVERLAPPED flag. I'll shortly be starting a rewrite of part of the RPC server so that we can support more transports and I'll bear this bug in mind when I do it and I'll try to fix it. The solution I come to will probably involve a worker thread doing the ConnectNamedPipe. I hope the code gets fixed before the next wine release, because rpcrt4 currently deadlocks (or should deadlock) when starting a server (at least in ROS/Win). If what you say is true about the PIPE_NOWAIT flag affecting this then it should be easy to fix. P.S. With WINE 0.9.5's implementation I noticed a massive thread leak on server-side (one thread per request?), is this a known bug? No. I'm pretty sure that the code works correctly and doesn't leak threads in Wine. If you can get a backtrace of the threads that shouldn't be around then I might be able to determine what is going wrong. -- Rob Shearman
Re: [rpcrt4] Properly created named pipes
Robert Shearman wrote: > I have a patch that converts rpcrt4 over to using overlapped I/O, but I > didn't submit it because the performance on Wine is horrible. When using > overlapped I/O we have to perform several more server calls than when > using non-overlapped I/O. Also, I think that this patch is incorrect > because if you want to make the pipe overlapped then you have to fix up > all of the ReadFile calls to take an OVERLAPPED structure. That's true, the ReadFile calls would also have to use the OVERLAPPED structure, but that would only work if the pipes were created with the PIPE_NOWAIT flag. Otherwise ReadFile and WriteFile will (or better should) be still synchronous regardless of the FILE_FLAG_OVERLAPPED flag. > I'll shortly be starting a rewrite of part of the RPC server so that we > can support more transports and I'll bear this bug in mind when I do it > and I'll try to fix it. The solution I come to will probably involve a > worker thread doing the ConnectNamedPipe. I hope the code gets fixed before the next wine release, because rpcrt4 currently deadlocks (or should deadlock) when starting a server (at least in ROS/Win). P.S. With WINE 0.9.5's implementation I noticed a massive thread leak on server-side (one thread per request?), is this a known bug? - Thomas
Re: [rpcrt4] Properly created named pipes
Thomas Weidenmueller wrote: Created the named pipes with the FILE_FLAG_OVERLAPPED flag, otherwise the call to ConnectNamedPipe() will block the server thread if no connection can be established, which causes the rpc server to dead-lock during startup. Hi Thomas, I have a patch that converts rpcrt4 over to using overlapped I/O, but I didn't submit it because the performance on Wine is horrible. When using overlapped I/O we have to perform several more server calls than when using non-overlapped I/O. Also, I think that this patch is incorrect because if you want to make the pipe overlapped then you have to fix up all of the ReadFile calls to take an OVERLAPPED structure. I'll shortly be starting a rewrite of part of the RPC server so that we can support more transports and I'll bear this bug in mind when I do it and I'll try to fix it. The solution I come to will probably involve a worker thread doing the ConnectNamedPipe. -- Rob Shearman
Re: [rpcrt4] Properly created named pipes
Oops...this meant to go to wine-patches Sorry about that. - Thomas
[rpcrt4] Properly created named pipes
Created the named pipes with the FILE_FLAG_OVERLAPPED flag, otherwise
the call to ConnectNamedPipe() will block the server thread if no
connection can be established, which causes the rpc server to dead-lock
during startup.
- Thomas
--
P.S.: Please let me know if there's something wrong with this patch or
tell me why it was rejected. Otherwise I'm going to assume the fixes
aren't appreciated or necessary because the implementation is considered
mature and stable.
Index: dlls/rpcrt4/rpc_binding.c
===
RCS file: /home/wine/wine/dlls/rpcrt4/rpc_binding.c,v
retrieving revision 1.39
diff -u -r1.39 rpc_binding.c
--- dlls/rpcrt4/rpc_binding.c 6 Sep 2005 10:26:14 - 1.39
+++ dlls/rpcrt4/rpc_binding.c 16 Jan 2006 23:00:15 -
@@ -142,7 +142,7 @@
pname = HeapAlloc(GetProcessHeap(), 0, strlen(prefix) +
strlen(Connection->Endpoint) + 1);
strcat(strcpy(pname, prefix), Connection->Endpoint);
TRACE("listening on %s\n", pname);
-Connection->conn = CreateNamedPipeA(pname, PIPE_ACCESS_DUPLEX,
+Connection->conn = CreateNamedPipeA(pname, PIPE_ACCESS_DUPLEX |
FILE_FLAG_OVERLAPPED,
PIPE_TYPE_MESSAGE |
PIPE_READMODE_MESSAGE, PIPE_UNLIMITED_INSTANCES,
RPC_MAX_PACKET_SIZE,
RPC_MAX_PACKET_SIZE, 5000, NULL);
HeapFree(GetProcessHeap(), 0, pname);
@@ -166,7 +166,7 @@
pname = HeapAlloc(GetProcessHeap(), 0, strlen(prefix) +
strlen(Connection->Endpoint) + 1);
strcat(strcpy(pname, prefix), Connection->Endpoint);
TRACE("listening on %s\n", pname);
-Connection->conn = CreateNamedPipeA(pname, PIPE_ACCESS_DUPLEX,
+Connection->conn = CreateNamedPipeA(pname, PIPE_ACCESS_DUPLEX |
FILE_FLAG_OVERLAPPED,
PIPE_TYPE_MESSAGE |
PIPE_READMODE_MESSAGE | PIPE_WAIT, PIPE_UNLIMITED_INSTANCES,
RPC_MAX_PACKET_SIZE,
RPC_MAX_PACKET_SIZE, 5000, NULL);
HeapFree(GetProcessHeap(), 0, pname);
