Re: Fix windows issue with multiple workers
Hi, enclosed you will find an amend fix as replacement to _sb-win-multi-worker-add-3.patch (just forgotten to save after renaming NGX_SINGLE_WORKER - NGX_CONF_UNSET_PTR, before it was commited). 18.06.2015 21:55, Maxim Dounin: As I already tried to explain, the approach with inherited sockets is basically identical to what nginx does on UNIX with fork(). There are no reasons to keep things different if you can do it similarly on both platforms. But it's not even roughly similar, see win32/ngx_process.c + win32/ngx_process_cycle.c in comparission to unix... This is already at all a biggish difference to unix. The goal is to minimize code bloat, not maximize it. Let me think a bit, how I can make it a little smaller, or unite some code-pieces with unix. Regards, sebres.# HG changeset patch # User sebres serg.bres...@sebres.de # Date 1434103966 -7200 # Fri Jun 12 12:12:46 2015 +0200 # Node ID b72d1091430e8899ee7d23c60924c100cba1c3ab # Parent 76ee2fe9300bdcf0dbf4a05e3ed7a1136b324eb7 backwards compatibility for single worker (without inheritance of socket) + code review / amend fix diff -r 76ee2fe9300b -r b72d1091430e src/core/ngx_connection.c --- a/src/core/ngx_connection.c Thu Jun 11 14:51:59 2015 +0200 +++ b/src/core/ngx_connection.c Fri Jun 12 12:12:46 2015 +0200 @@ -427,33 +427,33 @@ ngx_open_listening_sockets(ngx_cycle_t * /* try to use shared sockets of master */ if (ngx_process NGX_PROCESS_MASTER) { -if (!shinfo) { -shinfo = ngx_get_listening_share_info(cycle, ngx_getpid()); - -if (!shinfo) { -failed = 1; -break; -} +if (!shinfo ngx_get_listening_share_info(cycle, shinfo, +ngx_getpid()) != NGX_OK) { +failed = 1; +break; } -s = ngx_shared_socket(ls[i].sockaddr-sa_family, ls[i].type, 0, -shinfo+i); +if (shinfo != NGX_CONF_UNSET_PTR) { -if (s == (ngx_socket_t) -1) { -ngx_log_error(NGX_LOG_EMERG, log, ngx_socket_errno, - ngx_socket_n for inherited socket %V failed, - ls[i].addr_text); -return NGX_ERROR; +s = ngx_shared_socket(ls[i].sockaddr-sa_family, ls[i].type, 0, +shinfo+i); + +if (s == (ngx_socket_t) -1) { +ngx_log_error(NGX_LOG_EMERG, log, ngx_socket_errno, + ngx_socket_n for inherited socket %V failed, + ls[i].addr_text); +return NGX_ERROR; +} + +ngx_log_debug4(NGX_LOG_DEBUG_CORE, log, 0, [%d] shared socket %d %V: %d, +ngx_process, i, ls[i].addr_text, s); + +ls[i].fd = s; + +ls[i].listen = 1; + +continue; } - -ngx_log_debug4(NGX_LOG_DEBUG_CORE, log, 0, [%d] shared socket %d %V: %d, -ngx_process, i, ls[i].addr_text, s); - -ls[i].fd = s; - -ls[i].listen = 1; - -continue; } #endif diff -r 76ee2fe9300b -r b72d1091430e src/os/win32/ngx_process.c --- a/src/os/win32/ngx_process.c Thu Jun 11 14:51:59 2015 +0200 +++ b/src/os/win32/ngx_process.c Fri Jun 12 12:12:46 2015 +0200 @@ -70,9 +70,7 @@ ngx_spawn_process(ngx_cycle_t *cycle, ch return pid; } -#if (NGX_WIN32) ngx_share_listening_sockets(cycle, pid); -#endif ngx_memzero(ngx_processes[s], sizeof(ngx_process_t)); diff -r 76ee2fe9300b -r b72d1091430e src/os/win32/ngx_process_cycle.c --- a/src/os/win32/ngx_process_cycle.c Thu Jun 11 14:51:59 2015 +0200 +++ b/src/os/win32/ngx_process_cycle.c Fri Jun 12 12:12:46 2015 +0200 @@ -113,11 +113,6 @@ ngx_master_process_cycle(ngx_cycle_t *cy events[2] = ngx_reopen_event; events[3] = ngx_reload_event; -/* does not close listener for win32, will be shared */ -#if (!NGX_WIN32) -ngx_close_listening_sockets(cycle); -#endif - if (ngx_start_worker_processes(cycle, NGX_PROCESS_RESPAWN) == 0) { exit(2); } @@ -210,11 +205,6 @@ ngx_master_process_cycle(ngx_cycle_t *cy ngx_cycle = cycle; -/* does not close listener for win32, will be shared */ -#if (!NGX_WIN32) -ngx_close_listening_sockets(cycle); -#endif - if (ngx_start_worker_processes(cycle, NGX_PROCESS_JUST_RESPAWN)) { ngx_quit_worker_processes(cycle, 1); } diff -r 76ee2fe9300b -r b72d1091430e src/os/win32/ngx_socket.c --- a/src/os/win32/ngx_socket.c Thu Jun 11 14:51:59 2015 +0200 +++
Re: Fix windows issue with multiple workers
So, in VM it work for me also. I'm assuming that something on my windows work-pc has prevented to inherit listener in this way (driver, LSPs installed (Layered Service Providers), antivirus or something else)... But, why don't you want to use a suggested solution of me? If I will realize the way with easy inheritance (with bInheritHandle through CreateProcess), it will be not really easier, because: - we have several listener to share, so we should tell all this handles to child process; - bInheritHandle=True in CreateProcess can be a potential risk by not closed handles, if process crashed, and that are not only sockets - thus will arise so-called zombi-handles as half-open (dropped) or half-closed. But for sockets are listening it is extrem. Here is an example when this situation is encountered (* - listener, which process does not exist): netstat /ano | grep 0.0:80 * TCP0.0.0.0:80 0.0.0.0:0 LISTENING 3824 TCP0.0.0.0:80 0.0.0.0:0 LISTENING 4378 taskkill /F /PID 3824 ERROR: The process 3824 not found. Unfortunately, it is not guaranteed that new process 4378 accepts connections (because zombi listener of 3824 can block it). But also not so good are another zombies, like not closed temp-files, lock-files, pipes etc. You can talk long about that would be windows bugs, but that are facts. And thus it is instable. Apart from, does not work at all on some mashines (like my work-pc). And the way with WSADuplicateSocket self Microsoft recommends in various articles. If you still want to use the solution with bInheritHandle, I suggest a compromise: I will make it with selectable option (resp. defines like NGX_WIN32_DUPLICATE_LISTEN and NGX_WIN32_INHERIT_LISTEN). Please tell me your decision. Regards, sebres. Am 17.06.2015 16:52, schrieb Maxim Dounin: Hello! On Wed, Jun 17, 2015 at 04:01:17PM +0200, Sergey Brester wrote: Hmm, strange - almost same code, but it does not work... only first child can accept connections. Have you tried exactly the code I provided? Almost the same is a usual difference between working and non-working code. Which version of windows are you using for test? Works fine at least in Windows 7 and Windows 8.1 VMs here, both 32-bit. I have no 64-bit Windows on hand to test, but if it doesn't work for you specifically on 64-bit Windows, this may be some minor bug in the test code related to type casting. ___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: Fix windows issue with multiple workers
Hello! On Wed, Jun 17, 2015 at 09:58:46AM +0200, Sergey Brester wrote: Yes, for exactly one child process... The code in question uses the socket in two process, parent and child. For example, same (modified for multiprocessing) code does not work on my mashine (win7 x64) for 2nd (3th etc.) process (with error 10022 = INVARG). I think the handle schould be then duplicated, and see MSDN (https://msdn.microsoft.com/en-us/library/windows/desktop/ms724251.aspx) If something modified doesn't work for you - please be exact and provide the code which doesn't work. Below is the version of the code modified to accept connections in 6 processes simulteniously, and it works fine here. #include winsock2.h #include stdio.h #include windows.h #pragma comment(lib, Ws2_32.lib) int main(int argc, char *argv[]) { int rc, i; u_long code; SOCKET listen_socket, s; WSADATA wsaData; struct sockaddr_in sin; STARTUPINFO si; PROCESS_INFORMATION pi; char command[256]; rc = WSAStartup(MAKEWORD(2, 2), wsaData); if (rc != NO_ERROR) { printf(WSAStartup() failed: %d\n, rc); return 2; } if (argc == 2) { listen_socket = atoi(argv[1]); printf(Inherited socket: %d\n, listen_socket); goto accept; } if (argc != 1) { printf(Invalid number of arguments\n); return 1; } listen_socket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); if (listen_socket == INVALID_SOCKET) { printf(socket failed with error: %ld\n, WSAGetLastError()); return 1; } printf(Listen socket: %d\n, listen_socket); sin.sin_family = AF_INET; sin.sin_addr.s_addr = inet_addr(127.0.0.1); sin.sin_port = htons(8080); if (bind(listen_socket, (SOCKADDR *) sin, sizeof(sin)) == SOCKET_ERROR) { printf(bind() failed: %ld\n, WSAGetLastError()); return 1; } if (listen(listen_socket, 1) == SOCKET_ERROR) { printf(listen() failed: %ld\n, WSAGetLastError()); return 1; } for (i = 0; i 5; i++) { ZeroMemory(si, sizeof(si)); si.cb = sizeof(si); ZeroMemory(pi, sizeof(pi)); _snprintf(command, sizeof(command), %s %d, argv[0], listen_socket); if (CreateProcess(NULL, command, NULL, NULL, 1, 0, NULL, NULL, si, pi) == 0) { printf(CreateProcess() failed: %ld\n, GetLastError()); return 1; } #if 0 WaitForSingleObject(pi.hProcess, INFINITE); if (GetExitCodeProcess(pi.hProcess, code) == 0) { printf(GetExitCodeProcess() failed: %ld\n, GetLastError()); return 1; } printf(Child process exited: %d\n, code); #endif CloseHandle(pi.hProcess); CloseHandle(pi.hThread); } accept: printf(Waiting for client to connect...\n); s = accept(listen_socket, NULL, NULL); if (s == INVALID_SOCKET) { printf(accept() failed: %ld\n, WSAGetLastError()); return 1; } printf(Client connected\n); return 0; } -- Maxim Dounin http://nginx.org/ ___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: Fix windows issue with multiple workers
Hi, Yes, for exactly one child process... For example, same (modified for multiprocessing) code does not work on my mashine (win7 x64) for 2nd (3th etc.) process (with error 10022 = INVARG). I think the handle schould be then duplicated, and see MSDN (https://msdn.microsoft.com/en-us/library/windows/desktop/ms724251.aspx) Sockets. No error is returned, but the duplicate handle may not be recognized by Winsock at the target process. Also, using DuplicateHandle interferes with internal reference counting on the underlying object. To duplicate a socket handle, use the WSADuplicateSocket function. Regards, sebres. . Am 17.06.2015 04:27, schrieb Maxim Dounin: Hello! On Wed, Jun 10, 2015 at 09:48:28PM +0200, Sergey Brester wrote: [...] @Maxim Dounin: 1) your suggested way with shared handle and bInheritHandle does not work, because of: [quote] Sockets. No error is returned, but the duplicate handle may not be recognized by Winsock at the target process. Also, using DUPLICATEHANDLE interferes with internal reference counting on the underlying object. To duplicate a socket handle, use the WSADUPLICATESOCKET function. [/quote] The quote is from DuplicateHandle() description, which is irrelevant to the approach suggested. Sockets are inheritable between processes, including listen ones. Simple test code below, as adapted from the accept() MSDN example, demonstrates that the approach is working. #include winsock2.h #include stdio.h #include windows.h #pragma comment(lib, Ws2_32.lib) int main(int argc, char *argv[]) { int rc; u_long code; SOCKET listen_socket, s; WSADATA wsaData; struct sockaddr_in sin; STARTUPINFO si; PROCESS_INFORMATION pi; char command[256]; rc = WSAStartup(MAKEWORD(2, 2), wsaData); if (rc != NO_ERROR) { printf(WSAStartup() failed: %dn, rc); return 2; } if (argc == 2) { listen_socket = atoi(argv[1]); printf(Inherited socket: %dn, listen_socket); goto accept; } listen_socket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); if (listen_socket == INVALID_SOCKET) { printf(socket failed with error: %ldn, WSAGetLastError()); return 1; } printf(Listen socket: %dn, listen_socket); sin.sin_family = AF_INET; sin.sin_addr.s_addr = inet_addr(127.0.0.1); sin.sin_port = htons(8080); if (bind(listen_socket, (SOCKADDR *) sin, sizeof(sin)) == SOCKET_ERROR) { printf(bind() failed: %ldn, WSAGetLastError()); return 1; } if (listen(listen_socket, 1) == SOCKET_ERROR) { printf(listen() failed: %ldn, WSAGetLastError()); return 1; } if (argc == 1) { ZeroMemory(si, sizeof(si)); si.cb = sizeof(si); ZeroMemory(pi, sizeof(pi)); _snprintf(command, sizeof(command), %s %d, argv[0], listen_socket); if (CreateProcess(NULL, command, NULL, NULL, 1, 0, NULL, NULL, si, pi) == 0) { printf(CreateProcess() failed: %ldn, GetLastError()); return 1; } WaitForSingleObject(pi.hProcess, INFINITE); if (GetExitCodeProcess(pi.hProcess, code) == 0) { printf(GetExitCodeProcess() failed: %ldn, GetLastError()); return 1; } printf(Child process exited: %dn, code); CloseHandle(pi.hProcess); CloseHandle(pi.hThread); } accept: printf(Waiting for client to connect...n); s = accept(listen_socket, NULL, NULL); if (s == INVALID_SOCKET) { printf(accept() failed: %ldn, WSAGetLastError()); return 1; } printf(Client connectedn); return 0; } ___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: Fix windows issue with multiple workers
Hi, enclosed a further changeset with backwards compatibility to 1 worker processing (without inheritance as before fix), if single worker configured + a little bit code review. P.S. github updated also. Regards, sebres. 11.06.2015 15:03, Sergey Brester: Hi, I've forgotten to free the shmem, thus enclosed an amendment with clean-up, relative last changeset. Regards, sebres. # HG changeset patch # User sebres serg.bres...@sebres.de # Date 1434103966 -7200 # Fri Jun 12 12:12:46 2015 +0200 # Node ID 3499ecc86ec00deeef93869fc52a5ac02fd2e49f # Parent 76ee2fe9300bdcf0dbf4a05e3ed7a1136b324eb7 backwards compatibility for single worker (without inheritance of socket) + code review diff -r 76ee2fe9300b -r 3499ecc86ec0 src/core/ngx_connection.c --- a/src/core/ngx_connection.c Thu Jun 11 14:51:59 2015 +0200 +++ b/src/core/ngx_connection.c Fri Jun 12 12:12:46 2015 +0200 @@ -427,33 +427,33 @@ ngx_open_listening_sockets(ngx_cycle_t * /* try to use shared sockets of master */ if (ngx_process NGX_PROCESS_MASTER) { -if (!shinfo) { -shinfo = ngx_get_listening_share_info(cycle, ngx_getpid()); - -if (!shinfo) { -failed = 1; -break; -} +if (!shinfo ngx_get_listening_share_info(cycle, shinfo, +ngx_getpid()) != NGX_OK) { +failed = 1; +break; } -s = ngx_shared_socket(ls[i].sockaddr-sa_family, ls[i].type, 0, -shinfo+i); +if (shinfo != NGX_SINGLE_WORKER) { -if (s == (ngx_socket_t) -1) { -ngx_log_error(NGX_LOG_EMERG, log, ngx_socket_errno, - ngx_socket_n for inherited socket %V failed, - ls[i].addr_text); -return NGX_ERROR; +s = ngx_shared_socket(ls[i].sockaddr-sa_family, ls[i].type, 0, +shinfo+i); + +if (s == (ngx_socket_t) -1) { +ngx_log_error(NGX_LOG_EMERG, log, ngx_socket_errno, + ngx_socket_n for inherited socket %V failed, + ls[i].addr_text); +return NGX_ERROR; +} + +ngx_log_debug4(NGX_LOG_DEBUG_CORE, log, 0, [%d] shared socket %d %V: %d, +ngx_process, i, ls[i].addr_text, s); + +ls[i].fd = s; + +ls[i].listen = 1; + +continue; } - -ngx_log_debug4(NGX_LOG_DEBUG_CORE, log, 0, [%d] shared socket %d %V: %d, -ngx_process, i, ls[i].addr_text, s); - -ls[i].fd = s; - -ls[i].listen = 1; - -continue; } #endif diff -r 76ee2fe9300b -r 3499ecc86ec0 src/os/win32/ngx_process.c --- a/src/os/win32/ngx_process.c Thu Jun 11 14:51:59 2015 +0200 +++ b/src/os/win32/ngx_process.c Fri Jun 12 12:12:46 2015 +0200 @@ -70,9 +70,7 @@ ngx_spawn_process(ngx_cycle_t *cycle, ch return pid; } -#if (NGX_WIN32) ngx_share_listening_sockets(cycle, pid); -#endif ngx_memzero(ngx_processes[s], sizeof(ngx_process_t)); diff -r 76ee2fe9300b -r 3499ecc86ec0 src/os/win32/ngx_process_cycle.c --- a/src/os/win32/ngx_process_cycle.c Thu Jun 11 14:51:59 2015 +0200 +++ b/src/os/win32/ngx_process_cycle.c Fri Jun 12 12:12:46 2015 +0200 @@ -113,11 +113,6 @@ ngx_master_process_cycle(ngx_cycle_t *cy events[2] = ngx_reopen_event; events[3] = ngx_reload_event; -/* does not close listener for win32, will be shared */ -#if (!NGX_WIN32) -ngx_close_listening_sockets(cycle); -#endif - if (ngx_start_worker_processes(cycle, NGX_PROCESS_RESPAWN) == 0) { exit(2); } @@ -210,11 +205,6 @@ ngx_master_process_cycle(ngx_cycle_t *cy ngx_cycle = cycle; -/* does not close listener for win32, will be shared */ -#if (!NGX_WIN32) -ngx_close_listening_sockets(cycle); -#endif - if (ngx_start_worker_processes(cycle, NGX_PROCESS_JUST_RESPAWN)) { ngx_quit_worker_processes(cycle, 1); } diff -r 76ee2fe9300b -r 3499ecc86ec0 src/os/win32/ngx_socket.c --- a/src/os/win32/ngx_socket.c Thu Jun 11 14:51:59 2015 +0200 +++ b/src/os/win32/ngx_socket.c Fri Jun 12 12:12:46 2015 +0200 @@ -13,6 +13,7 @@ typedef struct { ngx_pid_t pid; ngx_uint_t nelts; +ngx_int_t worker_processes; /* WSAPROTOCOL_INFO * [listening.nelts] */ @@ -90,25 +91,30 @@ void ngx_free_listening_share(ngx_cycle_ } -ngx_shared_socket_info -ngx_get_listening_share_info(ngx_cycle_t *cycle, ngx_pid_t pid) +ngx_int_t
Re: Fix windows issue with multiple workers
Hi, I've forgotten to free the shmem, thus enclosed an amendment with clean-up, relative last changeset. Regards, sebres. 10.06.2015 21:48, Sergey Brester: Hi, enclosed you will find an attached changeset, that contains fix for windows issue with multiple workers (once listening - only one made any work). If someone needs a git version of it: https://github.com/sebres/nginx/pull/1/files [2] Here [3] you may find a benchmark comparison for that (1 worker column - measured before fix). -- Shortly about fix algorithm (changes related are marked as [*], unchanged - [-]): - master process create all listener; - [cycle] master process create a worker; * [win32] master calls `ngx_share_listening_sockets`: each listener share (inheritance) info for pid of this worker (cloned via WSADuplicateSocket), that will be saved in shmem; - master process wait until worker will send an event worker_nnn; * [win32] worker process executes `ngx_get_listening_share_info` to obtain shared info, protocol structure that can be used to create a new socket descriptor for a shared socket; * [win32] worker process creates all listener sockets using given shared info of master; - worker process sets an event worker_nnn; - master process create next worker, repeat [cycle]. -- @Maxim Dounin: 1) your suggested way with shared handle and bInheritHandle does not work, because of: [quote] Sockets. No error is returned, but the duplicate handle may not be recognized by Winsock at the target process. Also, using DUPLICATEHANDLE interferes with internal reference counting on the underlying object. To duplicate a socket handle, use the WSADUPLICATESOCKET function. [/quote] 2) proposal to use an environment instead of shared memory can not work also, because sharing via WSADuplicateSocket should already know a pid of target process, that uses this handle - specially shared for each worker. BTW, using of `accept_mutex` was disallowed for win32, cause of possible deadlock if grabbed by a process which can't accept connections. Because, this is fixed now, I have removed this restriction in separated commit. But I think, accept_mutex is not needed in win32 resp. with accept_mutex it is much slower as without him. So, whats about set default of `accept_mutex` to `off` on windows platform? BTW[2], I have executed extensive tests of this fix, also with reloading (increase/decrease `worker_processes`), restarting, as well as auto-restarting of worker, if it was crashed (ex.: have sporadically killed some worker). Regards, Serg G. Brester (aka sebres). ___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel [1] Links: -- [1] http://mailman.nginx.org/mailman/listinfo/nginx-devel [2] https://github.com/sebres/nginx/pull/1/files [3] https://github.com/sebres/nginx/pull/1 # HG changeset patch # User sebres serg.bres...@sebres.de # Date 1434027119 -7200 # Thu Jun 11 14:51:59 2015 +0200 # Node ID 76ee2fe9300bdcf0dbf4a05e3ed7a1136b324eb7 # Parent e40ee60150e47616d86fdee90f62f0f88c4b1e80 clean-up amendment for windows issue with multiple workers: free shared memory for inherited protocol info; diff -r e40ee60150e4 -r 76ee2fe9300b src/core/ngx_connection.c --- a/src/core/ngx_connection.c Wed Jun 10 19:39:18 2015 +0200 +++ b/src/core/ngx_connection.c Thu Jun 11 14:51:59 2015 +0200 @@ -641,6 +641,12 @@ ngx_open_listening_sockets(ngx_cycle_t * return NGX_ERROR; } +#if (NGX_WIN32) +if (ngx_process NGX_PROCESS_MASTER) { +ngx_free_listening_share(cycle); +} +#endif + return NGX_OK; } @@ -906,6 +912,10 @@ ngx_close_listening_sockets(ngx_cycle_t ngx_log_debug2(NGX_LOG_DEBUG_CORE, cycle-log, 0, [%d] close %d listener(s), ngx_process, cycle-listening.nelts); +#if (NGX_WIN32) +ngx_free_listening_share(cycle); +#endif + ngx_accept_mutex_held = 0; ngx_use_accept_mutex = 0; diff -r e40ee60150e4 -r 76ee2fe9300b src/os/win32/ngx_socket.c --- a/src/os/win32/ngx_socket.c Wed Jun 10 19:39:18 2015 +0200 +++ b/src/os/win32/ngx_socket.c Thu Jun 11 14:51:59 2015 +0200 @@ -79,6 +79,17 @@ ngx_int_t ngx_get_listening_share(ngx_cy } +void ngx_free_listening_share(ngx_cycle_t *cycle) +{ +if (shm_listener.addr) { + +ngx_shm_free(shm_listener); +shm_listener.addr = NULL; + +} +} + + ngx_shared_socket_info ngx_get_listening_share_info(ngx_cycle_t *cycle, ngx_pid_t pid) { diff -r e40ee60150e4 -r 76ee2fe9300b src/os/win32/ngx_socket.h --- a/src/os/win32/ngx_socket.h Wed Jun 10 19:39:18 2015 +0200 +++ b/src/os/win32/ngx_socket.h Thu Jun 11 14:51:59 2015 +0200 @@ -211,6 +211,7 @@ typedef WSAPROTOCOL_INFO * ngx_shared_so ngx_shared_socket_info ngx_get_listening_share_info(ngx_cycle_t *cycle, ngx_pid_t pid); +void