From 4e0e2ee1f5600a2a391d050d1bdeeccfa4ffd0ee Mon Sep 17 00:00:00 2001 From: Eli Lindsey Date: Thu, 18 Jun 2020 13:32:57 -0400 Subject: [PATCH] register only a single poll_t with libuv Most of the time we only need a read _or_ a write callback registered with libuv - for example, on a UDP request a write callback is registered, when executed the write callback performs the write, deregisters itself, and registers a read callback. However there is one case where getdns registers both read and write callbacks: when a backlog of TCP requests is going to the same upstream resolver, we use a single fd and queue the requests. In this instance we want to listen for both read (to get responses for requests we've already sent) and write (to continue to send our pending requests). libuv, like most event libraries, only allows one callback to be registered per fd. To get notification for both reads and writes, you should examine the event flags and have appropriate conditional logic within the single callback. Today getdns incorrectly tries to register two separate poll_t with libuv, one for read and one for write - this results in a crash (internal libuv assertion guaranteeing that only a single poll_t is registered per fd). This was tested by building locally, creating an ALIAS to a domain known to have large responses that truncate (forcing TCP), running trex with --resolver-cache-size 0, and flaming at 10qps. Previously this would crash within the first couple of seconds, now it works indefinitely. Note that a higher qps trigger a _different_ getdns/libuv crashing bug that occurs when the TCP backlog grows so large that requests start to time out. That crash is not addressed in this PR. --- src/extension/libuv.c | 72 ++++++++++++++++++------------------------- 1 file changed, 30 insertions(+), 42 deletions(-) diff --git a/src/extension/libuv.c b/src/extension/libuv.c index f1b7114e..6843a023 100644 --- a/src/extension/libuv.c +++ b/src/extension/libuv.c @@ -73,8 +73,7 @@ getdns_libuv_cleanup(getdns_eventloop *loop) } typedef struct poll_timer { - uv_poll_t read; - uv_poll_t write; + uv_poll_t poll; uv_timer_t timer; int to_close; struct mem_funcs mf; @@ -111,15 +110,8 @@ getdns_libuv_clear(getdns_eventloop *loop, getdns_eventloop_event *el_ev) DEBUG_UV("enter libuv_clear(el_ev = %p, my_ev = %p, to_close = %d)\n" , el_ev, my_ev, my_ev->to_close); - if (el_ev->read_cb) { - my_poll = &my_ev->read; - uv_poll_stop(my_poll); - my_ev->to_close += 1; - my_poll->data = my_ev; - uv_close((uv_handle_t *)my_poll, getdns_libuv_close_cb); - } - if (el_ev->write_cb) { - my_poll = &my_ev->write; + if (el_ev->read_cb || el_ev->write_cb) { + my_poll = &my_ev->poll; uv_poll_stop(my_poll); my_ev->to_close += 1; my_poll->data = my_ev; @@ -139,29 +131,28 @@ getdns_libuv_clear(getdns_eventloop *loop, getdns_eventloop_event *el_ev) } static void -getdns_libuv_read_cb(uv_poll_t *poll, int status, int events) +getdns_libuv_cb(uv_poll_t *poll, int status, int events) { - getdns_eventloop_event *el_ev = (getdns_eventloop_event *)poll->data; - (void)status; (void)events; - assert(el_ev->read_cb); - DEBUG_UV("enter libuv_read_cb(el_ev = %p, el_ev->ev = %p)\n" - , el_ev, el_ev->ev); - el_ev->read_cb(el_ev->userarg); - DEBUG_UV("exit libuv_read_cb(el_ev = %p, el_ev->ev = %p)\n" - , el_ev, el_ev->ev); -} + getdns_eventloop_event *el_ev = (getdns_eventloop_event *)poll->data; + (void)status; -static void -getdns_libuv_write_cb(uv_poll_t *poll, int status, int events) -{ - getdns_eventloop_event *el_ev = (getdns_eventloop_event *)poll->data; - (void)status; (void)events; - assert(el_ev->write_cb); - DEBUG_UV("enter libuv_write_cb(el_ev = %p, el_ev->ev = %p)\n" - , el_ev, el_ev->ev); - el_ev->write_cb(el_ev->userarg); - DEBUG_UV("exit libuv_write_cb(el_ev = %p, el_ev->ev = %p)\n" - , el_ev, el_ev->ev); + if (events & UV_READABLE) { + assert(el_ev->read_cb); + DEBUG_UV("enter libuv_read_cb(el_ev = %p, el_ev->ev = %p)\n" + , el_ev, el_ev->ev); + el_ev->read_cb(el_ev->userarg); + DEBUG_UV("exit libuv_read_cb(el_ev = %p, el_ev->ev = %p)\n" + , el_ev, el_ev->ev); + } else if (events & UV_WRITABLE) { + assert(el_ev->write_cb); + DEBUG_UV("enter libuv_write_cb(el_ev = %p, el_ev->ev = %p)\n" + , el_ev, el_ev->ev); + el_ev->write_cb(el_ev->userarg); + DEBUG_UV("exit libuv_write_cb(el_ev = %p, el_ev->ev = %p)\n" + , el_ev, el_ev->ev); + } else { + assert(ASSERT_UNREACHABLE); + } } static void @@ -205,18 +196,15 @@ getdns_libuv_schedule(getdns_eventloop *loop, my_ev->to_close = 0; my_ev->mf = ext->mf; el_ev->ev = my_ev; - - if (el_ev->read_cb) { - my_poll = &my_ev->read; + + if (el_ev->read_cb || el_ev->write_cb) { + my_poll = &my_ev->poll; my_poll->data = el_ev; uv_poll_init(ext->loop, my_poll, fd); - uv_poll_start(my_poll, UV_READABLE, getdns_libuv_read_cb); - } - if (el_ev->write_cb) { - my_poll = &my_ev->write; - my_poll->data = el_ev; - uv_poll_init(ext->loop, my_poll, fd); - uv_poll_start(my_poll, UV_WRITABLE, getdns_libuv_write_cb); + int events = + (el_ev->read_cb ? UV_READABLE : 0) | + (el_ev->write_cb ? UV_WRITABLE : 0); + uv_poll_start(my_poll, events, getdns_libuv_cb); } if (el_ev->timeout_cb) { my_timer = &my_ev->timer;