From 89b0883781532035546f48cd9edef92172ca54c9 Mon Sep 17 00:00:00 2001 From: shixuantong Date: Mon, 18 Sep 2023 16:20:46 +0800 Subject: [PATCH] eliminate redundant bev fd manipulating and caching (cherry picked from commit 19927002ade32c95d4ec7444c29422afa8bd87ef) --- ...oduce-bufferevent_replacefd-like-set.patch | 79 ++++ ...edundant-bev-fd-manipulating-and-cac.patch | 392 ++++++++++++++++++ ...-on-fd-reset-by-using-bufferevent_re.patch | 58 +++ libevent.spec | 8 +- 4 files changed, 536 insertions(+), 1 deletion(-) create mode 100644 backport-bufferevent-introduce-bufferevent_replacefd-like-set.patch create mode 100644 backport-http-eliminate-redundant-bev-fd-manipulating-and-cac.patch create mode 100644 backport-http-fix-fd-leak-on-fd-reset-by-using-bufferevent_re.patch diff --git a/backport-bufferevent-introduce-bufferevent_replacefd-like-set.patch b/backport-bufferevent-introduce-bufferevent_replacefd-like-set.patch new file mode 100644 index 0000000..65d4505 --- /dev/null +++ b/backport-bufferevent-introduce-bufferevent_replacefd-like-set.patch @@ -0,0 +1,79 @@ +From aea752b62dde0f02195b9c2143bdfcdfe65fb6fb Mon Sep 17 00:00:00 2001 +From: Azat Khuzhin +Date: Tue, 23 Mar 2021 09:00:24 +0300 +Subject: [PATCH] bufferevent: introduce bufferevent_replacefd() (like setfd() + but also close fd) + +Reference:https://github.com/libevent/libevent/commit/aea752b62dde0f02195b9c2143bdfcdfe65fb6fb +Conflict:NA +--- + bufferevent.c | 28 ++++++++++++++++++++++++++++ + include/event2/bufferevent.h | 12 ++++++++++++ + 2 files changed, 40 insertions(+) + +diff --git a/bufferevent.c b/bufferevent.c +index 08c0486..68b35b1 100644 +--- a/bufferevent.c ++++ b/bufferevent.c +@@ -876,6 +876,34 @@ bufferevent_setfd(struct bufferevent *bev, evutil_socket_t fd) + return res; + } + ++int ++bufferevent_replacefd(struct bufferevent *bev, evutil_socket_t fd) ++{ ++ union bufferevent_ctrl_data d; ++ int err = -1; ++ evutil_socket_t old_fd = EVUTIL_INVALID_SOCKET; ++ ++ BEV_LOCK(bev); ++ if (bev->be_ops->ctrl) { ++ err = bev->be_ops->ctrl(bev, BEV_CTRL_GET_FD, &d); ++ if (!err) { ++ old_fd = d.fd; ++ if (old_fd != EVUTIL_INVALID_SOCKET) { ++ err = evutil_closesocket(old_fd); ++ } ++ } ++ if (!err) { ++ d.fd = fd; ++ err = bev->be_ops->ctrl(bev, BEV_CTRL_SET_FD, &d); ++ } ++ } ++ if (err) ++ event_debug(("%s: cannot replace fd for %p from "EV_SOCK_FMT" to "EV_SOCK_FMT, __func__, bev, old_fd, fd)); ++ BEV_UNLOCK(bev); ++ ++ return err; ++} ++ + evutil_socket_t + bufferevent_getfd(struct bufferevent *bev) + { +diff --git a/include/event2/bufferevent.h b/include/event2/bufferevent.h +index 48cd153..e4e5c21 100644 +--- a/include/event2/bufferevent.h ++++ b/include/event2/bufferevent.h +@@ -355,6 +355,18 @@ void bufferevent_getcb(struct bufferevent *bufev, + EVENT2_EXPORT_SYMBOL + int bufferevent_setfd(struct bufferevent *bufev, evutil_socket_t fd); + ++/** ++ Replaces the file descriptor on which the bufferevent operates. ++ Not supported for all bufferevent types. ++ ++ Unlike bufferevent_setfd() it will close previous file descriptor (if any). ++ ++ @param bufev the bufferevent object for which to change the file descriptor ++ @param fd the file descriptor to operate on ++*/ ++EVENT2_EXPORT_SYMBOL ++int bufferevent_replacefd(struct bufferevent *bufev, evutil_socket_t fd); ++ + /** + Returns the file descriptor associated with a bufferevent, or -1 if + no file descriptor is associated with the bufferevent. +-- +2.33.0 + + diff --git a/backport-http-eliminate-redundant-bev-fd-manipulating-and-cac.patch b/backport-http-eliminate-redundant-bev-fd-manipulating-and-cac.patch new file mode 100644 index 0000000..b3c02c9 --- /dev/null +++ b/backport-http-eliminate-redundant-bev-fd-manipulating-and-cac.patch @@ -0,0 +1,392 @@ +From afa66ea4200a018bfc59abef8c2ffa11ef2b8363 Mon Sep 17 00:00:00 2001 +From: Azat Khuzhin +Date: Wed, 4 Sep 2019 00:56:20 +0300 +Subject: [PATCH] http: eliminate redundant bev fd manipulating and caching + [WIP] + +Reference:https://github.com/libevent/libevent/commit/afa66ea4200a018bfc59abef8c2ffa11ef2b8363 +Conflict:NA + +At the very beginning we reset the bufferevent fd (if bev has it), which +is not a good idea, since if user passes bufferevent with existing fd he +has some intention. + +So we need to: +- use BEV_OPT_CLOSE_ON_FREE for default bufferevent_socket_new() (to + avoid manual shutdown/closee) +- drop getsockopt(SOL_SOCKET, SO_ERROR), since bufferevent already has + evutil_socket_finished_connecting_() +- drop supperior bufferevent_setfd(bev, -1) in + evhttp_connection_connect_() + +Closes: #795 +Refs: #875 +--- + http-internal.h | 3 +- + http.c | 146 +++++++++++++++++------------------------- + include/event2/http.h | 6 +- + 3 files changed, 64 insertions(+), 91 deletions(-) + +diff --git a/http-internal.h b/http-internal.h +index 9f9b5ab..bf54d61 100644 +--- a/http-internal.h ++++ b/http-internal.h +@@ -53,7 +53,6 @@ struct evhttp_connection { + * server */ + TAILQ_ENTRY(evhttp_connection) next; + +- evutil_socket_t fd; + struct bufferevent *bufev; + + struct event retry_ev; /* for retrying connects */ +@@ -176,7 +175,7 @@ struct evhttp { + /* XXX most of these functions could be static. */ + + /* resets the connection; can be reused for more requests */ +-void evhttp_connection_reset_(struct evhttp_connection *); ++void evhttp_connection_reset_(struct evhttp_connection *, int); + + /* connects if necessary */ + int evhttp_connection_connect_(struct evhttp_connection *); +diff --git a/http.c b/http.c +index 785def9..551b63b 100644 +--- a/http.c ++++ b/http.c +@@ -777,7 +777,7 @@ evhttp_connection_fail_(struct evhttp_connection *evcon, + evhttp_request_free_(evcon, req); + + /* reset the connection */ +- evhttp_connection_reset_(evcon); ++ evhttp_connection_reset_(evcon, 1); + + /* We are trying the next request that was queued on us */ + if (TAILQ_FIRST(&evcon->requests) != NULL) +@@ -837,7 +837,7 @@ evhttp_connection_done(struct evhttp_connection *evcon) + + /* check if we got asked to close the connection */ + if (need_close) +- evhttp_connection_reset_(evcon); ++ evhttp_connection_reset_(evcon, 1); + + if (TAILQ_FIRST(&evcon->requests) != NULL) { + /* +@@ -1171,7 +1171,7 @@ evhttp_read_cb(struct bufferevent *bufev, void *arg) + __func__, EV_SIZE_ARG(total_len))); + #endif + +- evhttp_connection_reset_(evcon); ++ evhttp_connection_reset_(evcon, 1); + } + break; + case EVCON_DISCONNECTED: +@@ -1221,13 +1221,10 @@ void + evhttp_connection_free(struct evhttp_connection *evcon) + { + struct evhttp_request *req; +- int need_close = 0; + + /* notify interested parties that this connection is going down */ +- if (evcon->fd != -1) { +- if (evhttp_connected(evcon) && evcon->closecb != NULL) +- (*evcon->closecb)(evcon, evcon->closecb_arg); +- } ++ if (evhttp_connected(evcon) && evcon->closecb != NULL) ++ (*evcon->closecb)(evcon, evcon->closecb_arg); + + /* remove all requests that might be queued on this + * connection. for server connections, this should be empty. +@@ -1252,20 +1249,9 @@ evhttp_connection_free(struct evhttp_connection *evcon) + &evcon->read_more_deferred_cb); + + if (evcon->bufev != NULL) { +- need_close = +- !(bufferevent_get_options_(evcon->bufev) & BEV_OPT_CLOSE_ON_FREE); +- if (evcon->fd == -1) +- evcon->fd = bufferevent_getfd(evcon->bufev); +- + bufferevent_free(evcon->bufev); + } + +- if (evcon->fd != -1) { +- shutdown(evcon->fd, EVUTIL_SHUT_WR); +- if (need_close) +- evutil_closesocket(evcon->fd); +- } +- + if (evcon->bind_address != NULL) + mm_free(evcon->bind_address); + +@@ -1324,16 +1310,19 @@ evhttp_request_dispatch(struct evhttp_connection* evcon) + evhttp_write_buffer(evcon, evhttp_write_connectioncb, NULL); + } + +-/* Reset our connection state: disables reading/writing, closes our fd (if +-* any), clears out buffers, and puts us in state DISCONNECTED. */ +-void +-evhttp_connection_reset_(struct evhttp_connection *evcon) ++/** Hard-reset our connection state ++ * ++ * This will: ++ * - reset fd ++ * - clears out buffers ++ * - call closecb ++ */ ++static void ++evhttp_connection_reset_hard_(struct evhttp_connection *evcon) + { + struct evbuffer *tmp; + int err; + +- bufferevent_setcb(evcon->bufev, NULL, NULL, NULL, NULL); +- + /* XXXX This is not actually an optimal fix. Instead we ought to have + an API for "stop connecting", or use bufferevent_setfd to turn off + connecting. But for Libevent 2.0, this seems like a minimal change +@@ -1347,18 +1336,11 @@ evhttp_connection_reset_(struct evhttp_connection *evcon) + */ + bufferevent_disable_hard_(evcon->bufev, EV_READ|EV_WRITE); + +- if (evcon->fd == -1) +- evcon->fd = bufferevent_getfd(evcon->bufev); +- +- if (evcon->fd != -1) { +- /* inform interested parties about connection close */ +- if (evhttp_connected(evcon) && evcon->closecb != NULL) +- (*evcon->closecb)(evcon, evcon->closecb_arg); ++ /* inform interested parties about connection close */ ++ if (evhttp_connected(evcon) && evcon->closecb != NULL) ++ (*evcon->closecb)(evcon, evcon->closecb_arg); + +- shutdown(evcon->fd, EVUTIL_SHUT_WR); +- evutil_closesocket(evcon->fd); +- evcon->fd = -1; +- } ++ /** FIXME: manipulating with fd is unwanted */ + err = bufferevent_setfd(evcon->bufev, -1); + EVUTIL_ASSERT(!err && "setfd"); + +@@ -1369,9 +1351,26 @@ evhttp_connection_reset_(struct evhttp_connection *evcon) + tmp = bufferevent_get_input(evcon->bufev); + err = evbuffer_drain(tmp, -1); + EVUTIL_ASSERT(!err && "drain input"); ++} + +- evcon->flags &= ~EVHTTP_CON_READING_ERROR; ++/** Reset our connection state ++ * ++ * This will: ++ * - disables reading/writing ++ * - puts us in DISCONNECTED state ++ * ++ * @param hard - hard reset will (@see evhttp_connection_reset_hard_()) ++ */ ++void ++evhttp_connection_reset_(struct evhttp_connection *evcon, int hard) ++{ ++ bufferevent_setcb(evcon->bufev, NULL, NULL, NULL, NULL); + ++ if (hard) { ++ evhttp_connection_reset_hard_(evcon); ++ } ++ ++ evcon->flags &= ~EVHTTP_CON_READING_ERROR; + evcon->state = EVCON_DISCONNECTED; + } + +@@ -1403,7 +1402,7 @@ evhttp_connection_cb_cleanup(struct evhttp_connection *evcon) + { + struct evcon_requestq requests; + +- evhttp_connection_reset_(evcon); ++ evhttp_connection_reset_(evcon, 1); + if (evcon->retry_max < 0 || evcon->retry_cnt < evcon->retry_max) { + struct timeval tv_retry = evcon->initial_retry_timeout; + int i; +@@ -1481,16 +1480,13 @@ evhttp_error_cb(struct bufferevent *bufev, short what, void *arg) + struct evhttp_connection *evcon = arg; + struct evhttp_request *req = TAILQ_FIRST(&evcon->requests); + +- if (evcon->fd == -1) +- evcon->fd = bufferevent_getfd(bufev); +- + switch (evcon->state) { + case EVCON_CONNECTING: + if (what & BEV_EVENT_TIMEOUT) { + event_debug(("%s: connection timeout for \"%s:%d\" on " + EV_SOCK_FMT, + __func__, evcon->address, evcon->port, +- EV_SOCK_ARG(evcon->fd))); ++ EV_SOCK_ARG(bufferevent_getfd(bufev)))); + evhttp_connection_cb_cleanup(evcon); + return; + } +@@ -1526,7 +1522,7 @@ evhttp_error_cb(struct bufferevent *bufev, short what, void *arg) + * disconnected. + */ + EVUTIL_ASSERT(evcon->state == EVCON_IDLE); +- evhttp_connection_reset_(evcon); ++ evhttp_connection_reset_(evcon, 1); + + /* + * If we have no more requests that need completion +@@ -1572,11 +1568,6 @@ static void + evhttp_connection_cb(struct bufferevent *bufev, short what, void *arg) + { + struct evhttp_connection *evcon = arg; +- int error; +- ev_socklen_t errsz = sizeof(error); +- +- if (evcon->fd == -1) +- evcon->fd = bufferevent_getfd(bufev); + + if (!(what & BEV_EVENT_CONNECTED)) { + /* some operating systems return ECONNREFUSED immediately +@@ -1591,34 +1582,10 @@ evhttp_connection_cb(struct bufferevent *bufev, short what, void *arg) + return; + } + +- if (evcon->fd == -1) { +- event_debug(("%s: bufferevent_getfd returned -1", +- __func__)); +- goto cleanup; +- } +- +- /* Check if the connection completed */ +- if (getsockopt(evcon->fd, SOL_SOCKET, SO_ERROR, (void*)&error, +- &errsz) == -1) { +- event_debug(("%s: getsockopt for \"%s:%d\" on "EV_SOCK_FMT, +- __func__, evcon->address, evcon->port, +- EV_SOCK_ARG(evcon->fd))); +- goto cleanup; +- } +- +- if (error) { +- event_debug(("%s: connect failed for \"%s:%d\" on " +- EV_SOCK_FMT": %s", +- __func__, evcon->address, evcon->port, +- EV_SOCK_ARG(evcon->fd), +- evutil_socket_error_to_string(error))); +- goto cleanup; +- } +- + /* We are connected to the server now */ + event_debug(("%s: connected to \"%s:%d\" on "EV_SOCK_FMT"\n", + __func__, evcon->address, evcon->port, +- EV_SOCK_ARG(evcon->fd))); ++ EV_SOCK_ARG(bufferevent_getfd(bufev)))); + + /* Reset the retry count as we were successful in connecting */ + evcon->retry_cnt = 0; +@@ -2280,7 +2247,7 @@ evhttp_read_firstline(struct evhttp_connection *evcon, + if (res == DATA_CORRUPTED || res == DATA_TOO_LONG) { + /* Error while reading, terminate */ + event_debug(("%s: bad header lines on "EV_SOCK_FMT"\n", +- __func__, EV_SOCK_ARG(evcon->fd))); ++ __func__, EV_SOCK_ARG(bufferevent_getfd(evcon->bufev)))); + evhttp_connection_fail_(evcon, EVREQ_HTTP_INVALID_HEADER); + return; + } else if (res == MORE_DATA_EXPECTED) { +@@ -2297,7 +2264,7 @@ evhttp_read_header(struct evhttp_connection *evcon, + struct evhttp_request *req) + { + enum message_read_status res; +- evutil_socket_t fd = evcon->fd; ++ evutil_socket_t fd = bufferevent_getfd(evcon->bufev); + + res = evhttp_parse_headers_(req, bufferevent_get_input(evcon->bufev)); + if (res == DATA_CORRUPTED || res == DATA_TOO_LONG) { +@@ -2388,7 +2355,6 @@ evhttp_connection_base_bufferevent_new(struct event_base *base, struct evdns_bas + goto error; + } + +- evcon->fd = -1; + evcon->port = port; + + evcon->max_headers_size = EV_SIZE_MAX; +@@ -2403,7 +2369,7 @@ evhttp_connection_base_bufferevent_new(struct event_base *base, struct evdns_bas + } + + if (bev == NULL) { +- if (!(bev = bufferevent_socket_new(base, -1, 0))) { ++ if (!(bev = bufferevent_socket_new(base, -1, BEV_OPT_CLOSE_ON_FREE))) { + event_warn("%s: bufferevent_socket_new failed", __func__); + goto error; + } +@@ -2571,24 +2537,30 @@ evhttp_connection_connect_(struct evhttp_connection *evcon) + if (evcon->state == EVCON_CONNECTING) + return (0); + +- evhttp_connection_reset_(evcon); ++ /* Do not do hard reset, since this will reset the fd, but someone may ++ * change some options for it (i.e. setsockopt(), #875) ++ * ++ * However don't think that this options will be preserved for all ++ * connection lifetime, they will be reseted in the following cases: ++ * - evhttp_connection_set_local_address() ++ * - evhttp_connection_set_local_port() ++ * - evhttp_connection_set_retries() ++ * */ ++ evhttp_connection_reset_(evcon, 0); + + EVUTIL_ASSERT(!(evcon->flags & EVHTTP_CON_INCOMING)); + evcon->flags |= EVHTTP_CON_OUTGOING; + + if (evcon->bind_address || evcon->bind_port) { +- evcon->fd = bind_socket( +- evcon->bind_address, evcon->bind_port, 0 /*reuse*/); +- if (evcon->fd == -1) { ++ int fd = bind_socket(evcon->bind_address, evcon->bind_port, ++ 0 /*reuse*/); ++ if (fd == -1) { + event_debug(("%s: failed to bind to \"%s\"", + __func__, evcon->bind_address)); + return (-1); + } + +- if (bufferevent_setfd(evcon->bufev, evcon->fd)) +- return (-1); +- } else { +- if (bufferevent_setfd(evcon->bufev, -1)) ++ if (bufferevent_setfd(evcon->bufev, fd)) + return (-1); + } + +@@ -2625,7 +2597,7 @@ evhttp_connection_connect_(struct evhttp_connection *evcon) + + if (ret < 0) { + evcon->state = old_state; +- event_sock_warn(evcon->fd, "%s: connection to \"%s\" failed", ++ event_sock_warn(bufferevent_getfd(evcon->bufev), "%s: connection to \"%s\" failed", + __func__, evcon->address); + /* some operating systems return ECONNREFUSED immediately + * when connecting to a local address. the cleanup is going +@@ -4273,8 +4245,6 @@ evhttp_get_request_connection( + evcon->flags |= EVHTTP_CON_INCOMING; + evcon->state = EVCON_READING_FIRSTLINE; + +- evcon->fd = fd; +- + if (bufferevent_setfd(evcon->bufev, fd)) + goto err; + if (bufferevent_enable(evcon->bufev, EV_READ)) +diff --git a/include/event2/http.h b/include/event2/http.h +index ed9acf4..c1521ac 100644 +--- a/include/event2/http.h ++++ b/include/event2/http.h +@@ -739,7 +739,11 @@ void evhttp_connection_free(struct evhttp_connection *evcon); + EVENT2_EXPORT_SYMBOL + void evhttp_connection_free_on_completion(struct evhttp_connection *evcon); + +-/** sets the ip address from which http connections are made */ ++/** Sets the IP address from which http connections are made ++ * ++ * Note this resets internal bufferevent fd, so any options that had been ++ * installed will be flushed. ++ */ + EVENT2_EXPORT_SYMBOL + void evhttp_connection_set_local_address(struct evhttp_connection *evcon, + const char *address); +-- +2.33.0 + + diff --git a/backport-http-fix-fd-leak-on-fd-reset-by-using-bufferevent_re.patch b/backport-http-fix-fd-leak-on-fd-reset-by-using-bufferevent_re.patch new file mode 100644 index 0000000..d0f7c83 --- /dev/null +++ b/backport-http-fix-fd-leak-on-fd-reset-by-using-bufferevent_re.patch @@ -0,0 +1,58 @@ +From 2385638edf9cb833ebc2759cdb6d6d45dc51a0da Mon Sep 17 00:00:00 2001 +From: Azat Khuzhin +Date: Tue, 23 Mar 2021 09:02:39 +0300 +Subject: [PATCH] http: fix fd leak on fd reset (by using + bufferevent_replacefd()) + +Reference:https://github.com/libevent/libevent/commit/2385638edf9cb833ebc2759cdb6d6d45dc51a0da +Conflict:NA + +Fixes: afa66ea4 ("http: eliminate redundant bev fd manipulating and caching [WIP]") +--- + http.c | 8 ++++---- + 1 file changed, 4 insertions(+), 4 deletions(-) + +diff --git a/http.c b/http.c +index 551b63b..420049a 100644 +--- a/http.c ++++ b/http.c +@@ -1324,7 +1324,7 @@ evhttp_connection_reset_hard_(struct evhttp_connection *evcon) + int err; + + /* XXXX This is not actually an optimal fix. Instead we ought to have +- an API for "stop connecting", or use bufferevent_setfd to turn off ++ an API for "stop connecting", or use bufferevent_replacefd to turn off + connecting. But for Libevent 2.0, this seems like a minimal change + least likely to disrupt the rest of the bufferevent and http code. + +@@ -1341,7 +1341,7 @@ evhttp_connection_reset_hard_(struct evhttp_connection *evcon) + (*evcon->closecb)(evcon, evcon->closecb_arg); + + /** FIXME: manipulating with fd is unwanted */ +- err = bufferevent_setfd(evcon->bufev, -1); ++ err = bufferevent_replacefd(evcon->bufev, -1); + EVUTIL_ASSERT(!err && "setfd"); + + /* we need to clean up any buffered data */ +@@ -2560,7 +2560,7 @@ evhttp_connection_connect_(struct evhttp_connection *evcon) + return (-1); + } + +- if (bufferevent_setfd(evcon->bufev, fd)) ++ if (bufferevent_replacefd(evcon->bufev, fd)) + return (-1); + } + +@@ -4245,7 +4245,7 @@ evhttp_get_request_connection( + evcon->flags |= EVHTTP_CON_INCOMING; + evcon->state = EVCON_READING_FIRSTLINE; + +- if (bufferevent_setfd(evcon->bufev, fd)) ++ if (bufferevent_replacefd(evcon->bufev, fd)) + goto err; + if (bufferevent_enable(evcon->bufev, EV_READ)) + goto err; +-- +2.33.0 + + diff --git a/libevent.spec b/libevent.spec index 2d02730..383c2ad 100644 --- a/libevent.spec +++ b/libevent.spec @@ -1,6 +1,6 @@ Name: libevent Version: 2.1.12 -Release: 8 +Release: 9 Summary: An event notification library License: BSD @@ -19,6 +19,9 @@ Patch2: add-testcases-for-event.c-apis.patch # https://github.com/transmission/transmission/issues/1437 Patch3: 0001-Revert-Fix-checking-return-value-of-the-evdns_base_r.patch Patch6000: backport-ssl-do-not-trigger-EOF-if-some-data-had-been-successf.patch +Patch6001: backport-http-eliminate-redundant-bev-fd-manipulating-and-cac.patch +Patch6002: backport-http-fix-fd-leak-on-fd-reset-by-using-bufferevent_re.patch +Patch6003: backport-bufferevent-introduce-bufferevent_replacefd-like-set.patch %description Libevent additionally provides a sophisticated framework for buffered network IO, with support for sockets, @@ -79,6 +82,9 @@ rm -f %{buildroot}%{_libdir}/*.la %changelog +* Mon Sep 18 2023 shixuantong - 2.1.12-9 +- eliminate redundant bev fd manipulating and caching + * Sat Jul 29 2023 shixuantong - 2.1.12-8 - ssl: do not trigger EOF if some data had been successfully read -- Gitee