http: eliminate redundant bev fd manipulating and caching [WIP]

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

diff --git a/http-internal.h b/http-internal.h
index 2d0ae8fc69a5a09d7ee4c197ec476eab706b5c45..dfd5b01bf658478d21873e9dba0a61a8345a797b 100644 (file)
--- a/http-internal.h
+++ b/http-internal.h
@@ -51,7 +51,6 @@ struct evhttp_connection {
         * server */
        TAILQ_ENTRY(evhttp_connection) next;
 
-       evutil_socket_t fd;
        struct bufferevent *bufev;
 
        struct event retry_ev;          /* for retrying connects */
@@ -190,7 +189,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 45441f22b8f4cb0a824e76101d623e4bfe0bb93e..563d211aa3249ddb8cbe4338fc1e238f457e0c0c 100644 (file)
--- a/http.c
+++ b/http.c
@@ -872,7 +872,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)
@@ -932,7 +932,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) {
                        /*
@@ -1266,7 +1266,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:
@@ -1316,13 +1316,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.
@@ -1348,20 +1345,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);
 
@@ -1420,16 +1406,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
@@ -1443,18 +1432,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");
 
@@ -1465,9 +1447,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;
 }
 
@@ -1500,7 +1499,7 @@ evhttp_connection_cb_cleanup(struct evhttp_connection *evcon)
        struct evcon_requestq requests;
        EVUTIL_ASSERT(evcon->flags & EVHTTP_CON_OUTGOING);
 
-       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;
@@ -1585,16 +1584,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;
                }
@@ -1630,7 +1626,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
@@ -1676,11 +1672,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
@@ -1695,34 +1686,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;
@@ -2451,7 +2418,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) {
@@ -2468,7 +2435,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) {
@@ -2559,7 +2526,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;
@@ -2578,7 +2544,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;
                }
@@ -2777,24 +2743,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);
        }
 
@@ -2827,7 +2799,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
@@ -4523,8 +4495,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 d32bd0b35928b81fa9896c96bfd41c589dfb1d9d..22e6122e6b90a82791b3f961427a6e3f0249627b 100644 (file)
--- a/include/event2/http.h
+++ b/include/event2/http.h
@@ -869,7 +869,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);