From 4386f0971e4001227732837c6ec60f8e267f9be3 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 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.c | 232 ++++++++++++++++++++++-------------------- include/event2/http.h | 6 +- 2 files changed, 124 insertions(+), 114 deletions(-) diff --git a/http.c b/http.c index 04f089bc..a6b3f6bb 100644 --- a/http.c +++ b/http.c @@ -634,6 +634,91 @@ evhttp_make_header(struct evhttp_connection *evcon, struct evhttp_request *req) } } +/** + 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 +*/ +static 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; +} + +/** 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_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. + + Why is this here? If the fd is set in the bufferevent, and the + bufferevent is connecting, then you can't actually stop the + bufferevent from trying to connect with bufferevent_disable(). The + connect will never trigger, since we close the fd, but the timeout + might. That caused an assertion failure in evhttp_connection_fail_. + */ + bufferevent_disable_hard_(evcon->bufev, EV_READ|EV_WRITE); + + /* inform interested parties about connection close */ + if (evhttp_connected(evcon) && evcon->closecb != NULL) + (*evcon->closecb)(evcon, evcon->closecb_arg); + + /** FIXME: manipulating with fd is unwanted */ + err = bufferevent_replacefd(evcon->bufev, -1); + EVUTIL_ASSERT(!err && "setfd"); + + /* we need to clean up any buffered data */ + tmp = bufferevent_get_output(evcon->bufev); + err = evbuffer_drain(tmp, -1); + EVUTIL_ASSERT(!err && "drain output"); + tmp = bufferevent_get_input(evcon->bufev); + err = evbuffer_drain(tmp, -1); + EVUTIL_ASSERT(!err && "drain input"); + + evcon->flags &= ~EVHTTP_CON_READING_ERROR; + evcon->state = EVCON_DISCONNECTED; +} + void evhttp_connection_set_max_headers_size(struct evhttp_connection *evcon, ev_ssize_t new_max_headers_size) @@ -777,7 +862,7 @@ evhttp_connection_fail_(struct evhttp_connection *evcon, evhttp_request_free_(evcon, req); /* reset the connection */ - evhttp_connection_reset_(evcon); + evhttp_connection_reset_hard_(evcon); /* We are trying the next request that was queued on us */ if (TAILQ_FIRST(&evcon->requests) != NULL) @@ -837,7 +922,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_hard_(evcon); if (TAILQ_FIRST(&evcon->requests) != NULL) { /* @@ -1171,7 +1256,7 @@ evhttp_read_cb(struct bufferevent *bufev, void *arg) __func__, EV_SIZE_ARG(total_len))); #endif - evhttp_connection_reset_(evcon); + evhttp_connection_reset_hard_(evcon); } break; case EVCON_DISCONNECTED: @@ -1221,13 +1306,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 +1334,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,54 +1395,17 @@ 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. */ +/** Reset our connection state + * + * This will: + * - disables reading/writing + * - puts us in DISCONNECTED state + */ void evhttp_connection_reset_(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 - least likely to disrupt the rest of the bufferevent and http code. - - Why is this here? If the fd is set in the bufferevent, and the - bufferevent is connecting, then you can't actually stop the - bufferevent from trying to connect with bufferevent_disable(). The - connect will never trigger, since we close the fd, but the timeout - might. That caused an assertion failure in evhttp_connection_fail_. - */ - 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); - - shutdown(evcon->fd, EVUTIL_SHUT_WR); - evutil_closesocket(evcon->fd); - evcon->fd = -1; - } - err = bufferevent_setfd(evcon->bufev, -1); - EVUTIL_ASSERT(!err && "setfd"); - - /* we need to clean up any buffered data */ - tmp = bufferevent_get_output(evcon->bufev); - err = evbuffer_drain(tmp, -1); - EVUTIL_ASSERT(!err && "drain output"); - tmp = bufferevent_get_input(evcon->bufev); - err = evbuffer_drain(tmp, -1); - EVUTIL_ASSERT(!err && "drain input"); - evcon->flags &= ~EVHTTP_CON_READING_ERROR; - evcon->state = EVCON_DISCONNECTED; } @@ -1403,7 +1437,8 @@ evhttp_connection_cb_cleanup(struct evhttp_connection *evcon) { struct evcon_requestq requests; - evhttp_connection_reset_(evcon); + evhttp_connection_reset_hard_(evcon); + if (evcon->retry_max < 0 || evcon->retry_cnt < evcon->retry_max) { struct timeval tv_retry = evcon->initial_retry_timeout; int i; @@ -1481,16 +1516,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 +1558,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_hard_(evcon); /* * If we have no more requests that need completion @@ -1572,11 +1604,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 +1618,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 +2283,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 +2300,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 +2391,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 +2405,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 +2573,30 @@ evhttp_connection_connect_(struct evhttp_connection *evcon) if (evcon->state == EVCON_CONNECTING) return (0); + /* 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); 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_replacefd(evcon->bufev, fd)) return (-1); } @@ -2625,7 +2633,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 @@ -4265,9 +4273,7 @@ evhttp_get_request_connection( evcon->flags |= EVHTTP_CON_INCOMING; evcon->state = EVCON_READING_FIRSTLINE; - evcon->fd = fd; - - if (bufferevent_setfd(evcon->bufev, fd)) + if (bufferevent_replacefd(evcon->bufev, fd)) goto err; if (bufferevent_enable(evcon->bufev, EV_READ)) goto err; diff --git a/include/event2/http.h b/include/event2/http.h index 2a41303e..90f4cf9a 100644 --- a/include/event2/http.h +++ b/include/event2/http.h @@ -721,7 +721,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.46.0