From 636a13b8c46c52413e1da1795a952bfc738f3c55 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Thu, 23 May 2013 07:16:00 +0200 Subject: [PATCH] unix: fix stream refcounting buglet Fix a buglet where uv_read_stop() would mark the handle as stopped even when there are in-progress write requests. This bug is unlikely to have affected anyone, the only case where it has a user-visible effect is when: a) the handle has been stopped for reading but not writing, and b) it's the last active handle in the event loop's pollset If both conditions are met, it's possible for the event loop to terminate prematurely. This reapplies commit 80f2f82 which was temporarily reverted in fe7b154 because it was making a lot of node.js tests fail on OS X with the following assertion: Assertion failed: (!uv__is_active(handle)), function uv__finish_close, file ../../deps/uv/src/unix/core.c, line 165. Expecting that the handle is inactive when the state is UV_CLOSING turns out to be a bad assumption: it's possible that the handle is executing (for example) a shutdown request when uv__finish_close() is called. That's okay, uv__stream_destroy() takes care of that. The issue wasn't specific to OS X, it was just more visible on that platform. (Slow) debug builds on Linux exhibited the same behavior. --- src/unix/core.c | 7 ++++++- src/unix/stream.c | 10 ++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/unix/core.c b/src/unix/core.c index 7ab9bd6c..b7f0da38 100644 --- a/src/unix/core.c +++ b/src/unix/core.c @@ -162,7 +162,12 @@ void uv__make_close_pending(uv_handle_t* handle) { static void uv__finish_close(uv_handle_t* handle) { - assert(!uv__is_active(handle)); + /* Note: while the handle is in the UV_CLOSING state now, it's still possible + * for it to be active in the sense that uv__is_active() returns true. + * A good example is when the user calls uv_shutdown(), immediately followed + * by uv_close(). The handle is considered active at this point because the + * completion of the shutdown req is still pending. + */ assert(handle->flags & UV_CLOSING); assert(!(handle->flags & UV_CLOSED)); handle->flags |= UV_CLOSED; diff --git a/src/unix/stream.c b/src/unix/stream.c index aeefa2c4..fb1be005 100644 --- a/src/unix/stream.c +++ b/src/unix/stream.c @@ -429,6 +429,11 @@ void uv__stream_destroy(uv_stream_t* stream) { } if (stream->shutdown_req) { + /* The UV_ECANCELED error code is a lie, the shutdown(2) syscall is a + * fait accompli at this point. Maybe we should revisit this in v0.11. + * A possible reason for leaving it unchanged is that it informs the + * callee that the handle has been destroyed. + */ uv__req_unregister(stream->loop, stream->shutdown_req); uv__set_artificial_error(stream->loop, UV_ECANCELED); stream->shutdown_req->cb(stream->shutdown_req, -1); @@ -1318,9 +1323,10 @@ int uv_read_stop(uv_stream_t* stream) { stream->shutdown_req != NULL || stream->connect_req != NULL); - uv__io_stop(stream->loop, &stream->io_watcher, UV__POLLIN); - uv__handle_stop(stream); stream->flags &= ~UV_STREAM_READING; + uv__io_stop(stream->loop, &stream->io_watcher, UV__POLLIN); + if (!uv__io_active(&stream->io_watcher, UV__POLLOUT)) + uv__handle_stop(stream); #if defined(__APPLE__) /* Notify select() thread about state change */