From b2614a10a579ae6e9936eb483fe5ce14aa65a212 Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Wed, 24 Nov 2021 19:25:47 -0500 Subject: [PATCH] stream: permit read after seeing EOF (#3361) On some streams (notably TTYs), it is permitted to continue reading after getting EOF. So still stop reading on EOF, but allow the user to reset the stream and try to read again (which may just get EOF). This relaxes the constraint added in ce15b8405e9d01e221f8390475deab4a40d26e38. Refs: https://github.com/libuv/libuv/pull/3006 --- CMakeLists.txt | 2 +- Makefile.am | 2 +- src/unix/stream.c | 10 ++-- src/win/pipe.c | 1 - src/win/tcp.c | 3 +- test/test-list.h | 6 +-- ...adable-on-eof.c => test-readable-on-eof.c} | 52 +++++++++++-------- 7 files changed, 39 insertions(+), 37 deletions(-) rename test/{test-not-readable-on-eof.c => test-readable-on-eof.c} (68%) diff --git a/CMakeLists.txt b/CMakeLists.txt index 337b09c2..1d033f82 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -502,7 +502,6 @@ if(LIBUV_BUILD_TESTS) test/test-multiple-listen.c test/test-mutexes.c test/test-not-readable-nor-writable-on-read-error.c - test/test-not-readable-on-eof.c test/test-not-writable-after-shutdown.c test/test-osx-select.c test/test-pass-always.c @@ -530,6 +529,7 @@ if(LIBUV_BUILD_TESTS) test/test-process-title.c test/test-queue-foreach-delete.c test/test-random.c + test/test-readable-on-eof.c test/test-ref.c test/test-run-nowait.c test/test-run-once.c diff --git a/Makefile.am b/Makefile.am index cb3b42c2..3a2fba6d 100644 --- a/Makefile.am +++ b/Makefile.am @@ -206,7 +206,6 @@ test_run_tests_SOURCES = test/blackhole-server.c \ test/test-multiple-listen.c \ test/test-mutexes.c \ test/test-not-readable-nor-writable-on-read-error.c \ - test/test-not-readable-on-eof.c \ test/test-not-writable-after-shutdown.c \ test/test-osx-select.c \ test/test-pass-always.c \ @@ -234,6 +233,7 @@ test_run_tests_SOURCES = test/blackhole-server.c \ test/test-process-title-threadsafe.c \ test/test-queue-foreach-delete.c \ test/test-random.c \ + test/test-readable-on-eof.c \ test/test-ref.c \ test/test-run-nowait.c \ test/test-run-once.c \ diff --git a/src/unix/stream.c b/src/unix/stream.c index bc64fe8f..5858258d 100644 --- a/src/unix/stream.c +++ b/src/unix/stream.c @@ -1010,7 +1010,6 @@ uv_handle_type uv__handle_type(int fd) { static void uv__stream_eof(uv_stream_t* stream, const uv_buf_t* buf) { stream->flags |= UV_HANDLE_READ_EOF; stream->flags &= ~UV_HANDLE_READING; - stream->flags &= ~UV_HANDLE_READABLE; uv__io_stop(stream->loop, &stream->io_watcher, POLLIN); uv__handle_stop(stream); uv__stream_osx_interrupt_select(stream); @@ -1550,15 +1549,12 @@ int uv__read_start(uv_stream_t* stream, assert(stream->type == UV_TCP || stream->type == UV_NAMED_PIPE || stream->type == UV_TTY); - /* The UV_HANDLE_READING flag is irrelevant of the state of the tcp - it just - * expresses the desired state of the user. - */ + /* The UV_HANDLE_READING flag is irrelevant of the state of the stream - it + * just expresses the desired state of the user. */ stream->flags |= UV_HANDLE_READING; + stream->flags &= ~UV_HANDLE_READ_EOF; /* TODO: try to do the read inline? */ - /* TODO: keep track of tcp state. If we've gotten a EOF then we should - * not start the IO watcher. - */ assert(uv__stream_fd(stream) >= 0); assert(alloc_cb); diff --git a/src/win/pipe.c b/src/win/pipe.c index 912aed9b..984b766b 100644 --- a/src/win/pipe.c +++ b/src/win/pipe.c @@ -1796,7 +1796,6 @@ static void uv_pipe_read_eof(uv_loop_t* loop, uv_pipe_t* handle, * it. */ eof_timer_destroy(handle); - handle->flags &= ~UV_HANDLE_READABLE; uv_read_stop((uv_stream_t*) handle); handle->read_cb((uv_stream_t*) handle, UV_EOF, &buf); diff --git a/src/win/tcp.c b/src/win/tcp.c index cf2dbd85..be4133a8 100644 --- a/src/win/tcp.c +++ b/src/win/tcp.c @@ -1044,7 +1044,6 @@ void uv_process_tcp_read_req(uv_loop_t* loop, uv_tcp_t* handle, handle->flags &= ~UV_HANDLE_READING; DECREASE_ACTIVE_COUNT(loop, handle); } - handle->flags &= ~UV_HANDLE_READABLE; buf.base = 0; buf.len = 0; @@ -1081,7 +1080,7 @@ void uv_process_tcp_read_req(uv_loop_t* loop, uv_tcp_t* handle, } } else { /* Connection closed */ - handle->flags &= ~(UV_HANDLE_READING | UV_HANDLE_READABLE); + handle->flags &= ~UV_HANDLE_READING; DECREASE_ACTIVE_COUNT(loop, handle); handle->read_cb((uv_stream_t*)handle, UV_EOF, &buf); diff --git a/test/test-list.h b/test/test-list.h index 69a63ac5..3141a7cb 100644 --- a/test/test-list.h +++ b/test/test-list.h @@ -509,7 +509,7 @@ TEST_DECLARE (getters_setters) TEST_DECLARE (not_writable_after_shutdown) TEST_DECLARE (not_readable_nor_writable_on_read_error) -TEST_DECLARE (not_readable_on_eof) +TEST_DECLARE (readable_on_eof) #ifndef _WIN32 TEST_DECLARE (fork_timer) @@ -1145,8 +1145,8 @@ TASK_LIST_START TEST_HELPER (not_writable_after_shutdown, tcp4_echo_server) TEST_ENTRY (not_readable_nor_writable_on_read_error) TEST_HELPER (not_readable_nor_writable_on_read_error, tcp4_echo_server) - TEST_ENTRY (not_readable_on_eof) - TEST_HELPER (not_readable_on_eof, tcp4_echo_server) + TEST_ENTRY (readable_on_eof) + TEST_HELPER (readable_on_eof, tcp4_echo_server) TEST_ENTRY (metrics_idle_time) TEST_ENTRY (metrics_idle_time_thread) diff --git a/test/test-not-readable-on-eof.c b/test/test-readable-on-eof.c similarity index 68% rename from test/test-not-readable-on-eof.c rename to test/test-readable-on-eof.c index 2bb5f4ee..68e84542 100644 --- a/test/test-not-readable-on-eof.c +++ b/test/test-readable-on-eof.c @@ -35,7 +35,7 @@ static int close_cb_called; static void write_cb(uv_write_t* req, int status) { write_cb_called++; - ASSERT(status == 0); + ASSERT_EQ(status, 0); } static void alloc_cb(uv_handle_t* handle, @@ -51,12 +51,20 @@ static void close_cb(uv_handle_t* handle) { } static void read_cb(uv_stream_t* handle, ssize_t nread, const uv_buf_t* buf) { - read_cb_called++; + int r; - ASSERT(nread == UV_EOF); - ASSERT(0 == uv_is_readable(handle)); + ASSERT_EQ(nread, UV_EOF); + ASSERT_EQ(uv_is_readable(handle), 1); + ASSERT_EQ(uv_is_writable(handle), 1); - uv_close((uv_handle_t*) handle, close_cb); + if (++read_cb_called == 3) { + uv_close((uv_handle_t*) handle, close_cb); + ASSERT_EQ(uv_is_readable(handle), 0); + ASSERT_EQ(uv_is_writable(handle), 0); + } else { + r = uv_read_start((uv_stream_t*) &tcp_client, alloc_cb, read_cb); + ASSERT_EQ(r, 0); + } } static void connect_cb(uv_connect_t* req, int status) { @@ -64,10 +72,9 @@ static void connect_cb(uv_connect_t* req, int status) { uv_buf_t close_me; connect_cb_called++; - ASSERT(status == 0); + ASSERT_EQ(status, 0); - r = uv_read_start((uv_stream_t*) &tcp_client, alloc_cb, read_cb); - ASSERT(r == 0); + read_cb((uv_stream_t*) &tcp_client, UV_EOF, NULL); close_me = uv_buf_init(close_me_cmd, sizeof(close_me_cmd)); @@ -77,26 +84,27 @@ static void connect_cb(uv_connect_t* req, int status) { 1, write_cb); - ASSERT(r == 0); + ASSERT_EQ(r, 0); } -TEST_IMPL(not_readable_on_eof) { +TEST_IMPL(readable_on_eof) { struct sockaddr_in sa; - ASSERT(0 == uv_ip4_addr("127.0.0.1", TEST_PORT, &sa)); - ASSERT(0 == uv_loop_init(&loop)); - ASSERT(0 == uv_tcp_init(&loop, &tcp_client)); + ASSERT_EQ(uv_ip4_addr("127.0.0.1", TEST_PORT, &sa), 0); + ASSERT_EQ(uv_loop_init(&loop), 0); + ASSERT_EQ(uv_tcp_init(&loop, &tcp_client), 0); - ASSERT(0 == uv_tcp_connect(&connect_req, - &tcp_client, - (const struct sockaddr*) &sa, - connect_cb)); + ASSERT_EQ(uv_tcp_connect(&connect_req, + &tcp_client, + (const struct sockaddr*) &sa, + connect_cb), + 0); - ASSERT(0 == uv_run(&loop, UV_RUN_DEFAULT)); + ASSERT_EQ(uv_run(&loop, UV_RUN_DEFAULT), 0); - ASSERT(connect_cb_called == 1); - ASSERT(read_cb_called == 1); - ASSERT(write_cb_called == 1); - ASSERT(close_cb_called == 1); + ASSERT_EQ(connect_cb_called, 1); + ASSERT_EQ(read_cb_called, 3); + ASSERT_EQ(write_cb_called, 1); + ASSERT_EQ(close_cb_called, 1); MAKE_VALGRIND_HAPPY(); return 0;