From 88c2af0e65ed49ff599f5afd4bf952ecdfab4594 Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Tue, 14 Nov 2017 17:46:50 -0500 Subject: [PATCH] req: revisions to uv_req_t handling - Remove the `active_reqs` queue, which is never used. There are more efficient per-stream queues that `libuv` uses whenever it needs this information, so duplicating it and managing it here seems like unnecessary extra space and work. - Unix `uv_loop_init` didn't explicitly initialize. `loop->active_handles` (although it did memset the whole struct to 0, so it wasn't wrong previously, just inconsistent). - Consolidate repeated code for `uv__has_active_reqs`. - Change `uv__loop_alive` to use the helper functions (mirroring the unix copy of the same function). - Initialize some more uv_stream_t fields in init, rather than waiting for the connection callback. This helps surface bugs in libuv or the caller better, since it ensures libuv doesn't see uninitialized memory if asked to look at these fields before it thinks the socket is connected. - Fixes what appears to be a double-counting of `handle->reqs_pending`, in the highly-unlikely event that the code wants to emulate IOCP, but `RegisterWaitForSingleObject` somehow managed to fail. PR-URL: https://github.com/libuv/libuv/pull/1746 Reviewed-By: Ben Noordhuis Reviewed-By: Colin Ihrig Reviewed-By: Santiago Gimeno --- include/uv.h | 8 +++++--- src/unix/loop.c | 3 ++- src/uv-common.c | 2 +- src/uv-common.h | 6 +++--- src/win/core.c | 6 +++--- src/win/pipe.c | 1 + src/win/stream-inl.h | 10 +++++----- src/win/tcp.c | 2 -- 8 files changed, 20 insertions(+), 18 deletions(-) diff --git a/include/uv.h b/include/uv.h index 9794d996..5328373a 100644 --- a/include/uv.h +++ b/include/uv.h @@ -378,8 +378,7 @@ UV_EXTERN const char* uv_err_name(int err); /* read-only */ \ uv_req_type type; \ /* private */ \ - void* active_queue[2]; \ - void* reserved[4]; \ + void* reserved[6]; \ UV_REQ_PRIVATE_FIELDS \ /* Abstract base class of all requests. */ @@ -1531,7 +1530,10 @@ struct uv_loop_s { /* Loop reference counting. */ unsigned int active_handles; void* handle_queue[2]; - void* active_reqs[2]; + union { + void* unused[2]; + unsigned int count; + } active_reqs; /* Internal flag to signal loop stop. */ unsigned int stop_flag; UV_LOOP_PRIVATE_FIELDS diff --git a/src/unix/loop.c b/src/unix/loop.c index 5b5b0e09..99ead6cb 100644 --- a/src/unix/loop.c +++ b/src/unix/loop.c @@ -38,13 +38,14 @@ int uv_loop_init(uv_loop_t* loop) { heap_init((struct heap*) &loop->timer_heap); QUEUE_INIT(&loop->wq); - QUEUE_INIT(&loop->active_reqs); QUEUE_INIT(&loop->idle_handles); QUEUE_INIT(&loop->async_handles); QUEUE_INIT(&loop->check_handles); QUEUE_INIT(&loop->prepare_handles); QUEUE_INIT(&loop->handle_queue); + loop->active_handles = 0; + loop->active_reqs.count = 0; loop->nfds = 0; loop->watchers = NULL; loop->nwatchers = 0; diff --git a/src/uv-common.c b/src/uv-common.c index bc7d1379..71345895 100644 --- a/src/uv-common.c +++ b/src/uv-common.c @@ -627,7 +627,7 @@ int uv_loop_close(uv_loop_t* loop) { void* saved_data; #endif - if (!QUEUE_EMPTY(&(loop)->active_reqs)) + if (uv__has_active_reqs(loop)) return UV_EBUSY; QUEUE_FOREACH(q, &loop->handle_queue) { diff --git a/src/uv-common.h b/src/uv-common.h index d4fa22aa..c497d014 100644 --- a/src/uv-common.h +++ b/src/uv-common.h @@ -133,18 +133,18 @@ int uv__socket_sockopt(uv_handle_t* handle, int optname, int* value); void uv__fs_scandir_cleanup(uv_fs_t* req); #define uv__has_active_reqs(loop) \ - (QUEUE_EMPTY(&(loop)->active_reqs) == 0) + ((loop)->active_reqs.count > 0) #define uv__req_register(loop, req) \ do { \ - QUEUE_INSERT_TAIL(&(loop)->active_reqs, &(req)->active_queue); \ + (loop)->active_reqs.count++; \ } \ while (0) #define uv__req_unregister(loop, req) \ do { \ assert(uv__has_active_reqs(loop)); \ - QUEUE_REMOVE(&(req)->active_queue); \ + (loop)->active_reqs.count--; \ } \ while (0) diff --git a/src/win/core.c b/src/win/core.c index 9ed4e824..5fa9b666 100644 --- a/src/win/core.c +++ b/src/win/core.c @@ -239,7 +239,7 @@ int uv_loop_init(uv_loop_t* loop) { QUEUE_INIT(&loop->wq); QUEUE_INIT(&loop->handle_queue); - QUEUE_INIT(&loop->active_reqs); + loop->active_reqs.count = 0; loop->active_handles = 0; loop->pending_reqs_tail = NULL; @@ -470,8 +470,8 @@ static void uv_poll_ex(uv_loop_t* loop, DWORD timeout) { static int uv__loop_alive(const uv_loop_t* loop) { - return loop->active_handles > 0 || - !QUEUE_EMPTY(&loop->active_reqs) || + return uv__has_active_handles(loop) || + uv__has_active_reqs(loop) || loop->endgame_handles != NULL; } diff --git a/src/win/pipe.c b/src/win/pipe.c index 1a7c4dc1..0ecfbf1f 100644 --- a/src/win/pipe.c +++ b/src/win/pipe.c @@ -847,6 +847,7 @@ static void uv_pipe_queue_accept(uv_loop_t* loop, uv_pipe_t* handle, return; } + /* Wait for completion via IOCP */ handle->reqs_pending++; } diff --git a/src/win/stream-inl.h b/src/win/stream-inl.h index dba03747..40f5ddd5 100644 --- a/src/win/stream-inl.h +++ b/src/win/stream-inl.h @@ -37,11 +37,6 @@ INLINE static void uv_stream_init(uv_loop_t* loop, handle->write_queue_size = 0; handle->activecnt = 0; handle->stream.conn.shutdown_req = NULL; -} - - -INLINE static void uv_connection_init(uv_stream_t* handle) { - handle->flags |= UV_HANDLE_CONNECTION; handle->stream.conn.write_reqs_pending = 0; UV_REQ_INIT(&handle->read_req, UV_READ); @@ -51,4 +46,9 @@ INLINE static void uv_connection_init(uv_stream_t* handle) { } +INLINE static void uv_connection_init(uv_stream_t* handle) { + handle->flags |= UV_HANDLE_CONNECTION; +} + + #endif /* UV_WIN_STREAM_INL_H_ */ diff --git a/src/win/tcp.c b/src/win/tcp.c index fd6efbaf..f84a0d1e 100644 --- a/src/win/tcp.c +++ b/src/win/tcp.c @@ -459,8 +459,6 @@ static void uv_tcp_queue_accept(uv_tcp_t* handle, uv_tcp_accept_t* req) { INFINITE, WT_EXECUTEINWAITTHREAD)) { SET_REQ_ERROR(req, GetLastError()); uv_insert_pending_req(loop, (uv_req_t*)req); - handle->reqs_pending++; - return; } } else { /* Make this req pending reporting an error. */