From 4be6c70bb02e91903fb8ea5c0b1bd647c22d15e2 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Tue, 8 Sep 2015 15:44:13 +0300 Subject: [PATCH 1/3] test/regress_http: verify that closecb will be called without multiple write And now this works incorrect, i.e. http layer will not detect EOF until another write. Reported-in: #78 --- test/regress_http.c | 40 +++++++++++++++++++++++++++++++++------- 1 file changed, 33 insertions(+), 7 deletions(-) diff --git a/test/regress_http.c b/test/regress_http.c index 1a5ee6db..75dd41ed 100644 --- a/test/regress_http.c +++ b/test/regress_http.c @@ -3724,6 +3724,7 @@ struct terminate_state { struct bufferevent *bev; evutil_socket_t fd; int gotclosecb: 1; + int oneshot: 1; }; static void @@ -3731,7 +3732,10 @@ terminate_chunked_trickle_cb(evutil_socket_t fd, short events, void *arg) { struct terminate_state *state = arg; struct evbuffer *evb; - struct timeval tv; + + if (!state->req) { + return; + } if (evhttp_request_get_connection(state->req) == NULL) { test_ok = 1; @@ -3745,11 +3749,14 @@ terminate_chunked_trickle_cb(evutil_socket_t fd, short events, void *arg) evhttp_send_reply_chunk(state->req, evb); evbuffer_free(evb); - tv.tv_sec = 0; - tv.tv_usec = 3000; - EVUTIL_ASSERT(state); - EVUTIL_ASSERT(state->base); - event_base_once(state->base, -1, EV_TIMEOUT, terminate_chunked_trickle_cb, arg, &tv); + if (!state->oneshot) { + struct timeval tv; + tv.tv_sec = 0; + tv.tv_usec = 3000; + EVUTIL_ASSERT(state); + EVUTIL_ASSERT(state->base); + event_base_once(state->base, -1, EV_TIMEOUT, terminate_chunked_trickle_cb, arg, &tv); + } } static void @@ -3757,6 +3764,13 @@ terminate_chunked_close_cb(struct evhttp_connection *evcon, void *arg) { struct terminate_state *state = arg; state->gotclosecb = 1; + + /** TODO: though we can do this unconditionally */ + if (state->oneshot) { + evhttp_request_free(state->req); + state->req = NULL; + event_base_loopexit(state->base,NULL); + } } static void @@ -3796,7 +3810,7 @@ terminate_readcb(struct bufferevent *bev, void *arg) static void -http_terminate_chunked_test(void *arg) +http_terminate_chunked_test_impl(void *arg, int oneshot) { struct basic_test_data *data = arg; struct bufferevent *bev = NULL; @@ -3825,6 +3839,7 @@ http_terminate_chunked_test(void *arg) terminate_state.fd = fd; terminate_state.bev = bev; terminate_state.gotclosecb = 0; + terminate_state.oneshot = oneshot; /* first half of the http request */ http_request = @@ -3848,6 +3863,16 @@ http_terminate_chunked_test(void *arg) if (http) evhttp_free(http); } +static void +http_terminate_chunked_test(void *arg) +{ + http_terminate_chunked_test_impl(arg, 0); +} +static void +http_terminate_chunked_oneshot_test(void *arg) +{ + http_terminate_chunked_test_impl(arg, 1); +} static struct regress_dns_server_table ipv6_search_table[] = { { "localhost", "AAAA", "::1", 0 }, @@ -3998,6 +4023,7 @@ struct testcase_t http_testcases[] = { HTTP(incomplete), HTTP(incomplete_timeout), HTTP(terminate_chunked), + HTTP(terminate_chunked_oneshot), HTTP(on_complete), HTTP(highport), From 3d15aeb4fddb4ef150061eb1ddf11d66b5725960 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Wed, 9 Sep 2015 17:45:44 +0300 Subject: [PATCH 2/3] test/regress_http: cover write during read This is the regression for evhttp_write_buffer() where we reset readcb to avoid illegal state: http/write_during_read: [err] evhttp_read_cb: illegal connection state 7 If you will comment that this test will fail. --- test/regress_http.c | 67 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/test/regress_http.c b/test/regress_http.c index 75dd41ed..c86ec95f 100644 --- a/test/regress_http.c +++ b/test/regress_http.c @@ -84,6 +84,7 @@ static char const BASIC_REQUEST_BODY[] = "This is funny"; IMPL_HTTP_REQUEST_ERROR_CB(cancel, EVREQ_HTTP_REQUEST_CANCEL) static void http_basic_cb(struct evhttp_request *req, void *arg); +static void http_large_cb(struct evhttp_request *req, void *arg); static void http_chunked_cb(struct evhttp_request *req, void *arg); static void http_post_cb(struct evhttp_request *req, void *arg); static void http_put_cb(struct evhttp_request *req, void *arg); @@ -133,6 +134,7 @@ http_setup(ev_uint16_t *pport, struct event_base *base, int ipv6) /* Register a callback for certain types of requests */ evhttp_set_cb(myhttp, "/test", http_basic_cb, base); + evhttp_set_cb(myhttp, "/large", http_large_cb, base); evhttp_set_cb(myhttp, "/chunked", http_chunked_cb, base); evhttp_set_cb(myhttp, "/streamed", http_chunked_cb, base); evhttp_set_cb(myhttp, "/postit", http_post_cb, base); @@ -329,6 +331,19 @@ end: evbuffer_free(evb); } +static void +http_large_cb(struct evhttp_request *req, void *arg) +{ + struct evbuffer *evb = evbuffer_new(); + int i; + + for (i = 0; i < 1<<20; ++i) { + evbuffer_add_printf(evb, BASIC_REQUEST_BODY); + } + evhttp_send_reply(req, HTTP_OK, "Everything is fine", evb); + evbuffer_free(evb); +} + static char const* const CHUNKS[] = { "This is funny", "but not hilarious.", @@ -3990,6 +4005,56 @@ http_set_family_ipv6_test(void *arg) http_ipv6_for_domain_test_impl(arg, AF_INET6); } +static void +http_write_during_read(evutil_socket_t fd, short what, void *arg) +{ + struct bufferevent *bev = arg; + struct timeval tv; + + bufferevent_write(bev, "foobar", strlen("foobar")); + + evutil_timerclear(&tv); + tv.tv_sec = 1; + event_base_loopexit(exit_base, &tv); +} +static void +http_write_during_read_test(void *arg) +{ + struct basic_test_data *data = arg; + ev_uint16_t port = 0; + struct bufferevent *bev = NULL; + struct timeval tv; + int fd; + const char *http_request; + + test_ok = 0; + exit_base = data->base; + + http = http_setup(&port, data->base, 0); + + fd = http_connect("127.0.0.1", port); + bev = bufferevent_socket_new(data->base, fd, 0); + bufferevent_setcb(bev, NULL, NULL, NULL, data->base); + bufferevent_disable(bev, EV_READ); + + http_request = + "GET /large HTTP/1.1\r\n" + "Host: somehost\r\n" + "\r\n"; + + bufferevent_write(bev, http_request, strlen(http_request)); + evutil_timerclear(&tv); + tv.tv_usec = 10000; + event_base_once(data->base, -1, EV_TIMEOUT, http_write_during_read, bev, &tv); + + event_base_dispatch(data->base); + + if (bev) + bufferevent_free(bev); + if (http) + evhttp_free(http); +} + #define HTTP_LEGACY(name) \ { #name, run_legacy_test_fn, TT_ISOLATED|TT_LEGACY, &legacy_setup, \ http_##name##_test } @@ -4050,6 +4115,8 @@ struct testcase_t http_testcases[] = { HTTP(set_family_ipv4), HTTP(set_family_ipv6), + HTTP(write_during_read), + END_OF_TESTCASES }; From 7ed02ac12926c7a1b004d14652deabffb7575c20 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Tue, 8 Sep 2015 16:40:55 +0300 Subject: [PATCH 3/3] http: fix detecting EOF without write Before this patch http server don't knows when client disconnected until it will try to write to it, IOW to detect is client still alive you need to write something to client socket, however it is not convenient since it requires to store all clients somewhere and poll them periodically, and I don't see any regressions if we will leave EV_READ always (like libevhtp do), since we already reset read callback in evhttp_write_buffer() (see http/write_during_read). Also since we don't disable EV_READ anymore we don't need some enable EV_READ, so we will reduce number of epoll_ctl() calls. Covered-by: http/terminate_chunked_oneshot Covered-by: http/write_during_read Fixes: #78 --- http.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/http.c b/http.c index 2825b190..a30b25dd 100644 --- a/http.c +++ b/http.c @@ -969,7 +969,6 @@ evhttp_read_trailer(struct evhttp_connection *evcon, struct evhttp_request *req) case MORE_DATA_EXPECTED: case REQUEST_CANCELED: /* ??? */ default: - bufferevent_enable(evcon->bufev, EV_READ); break; } } @@ -1051,9 +1050,6 @@ evhttp_read_body(struct evhttp_connection *evcon, struct evhttp_request *req) evhttp_connection_done(evcon); return; } - - /* Read more! */ - bufferevent_enable(evcon->bufev, EV_READ); } #define get_deferred_queue(evcon) \ @@ -2180,9 +2176,6 @@ evhttp_read_header(struct evhttp_connection *evcon, return; } - /* Disable reading for now */ - bufferevent_disable(evcon->bufev, EV_READ); - /* Callback can shut down connection with negative return value */ if (req->header_cb != NULL) { if ((*req->header_cb)(req, req->cb_arg) < 0) { @@ -2595,9 +2588,9 @@ evhttp_cancel_request(struct evhttp_request *req) void evhttp_start_read_(struct evhttp_connection *evcon) { - /* Set up an event to read the headers */ bufferevent_disable(evcon->bufev, EV_WRITE); bufferevent_enable(evcon->bufev, EV_READ); + evcon->state = EVCON_READING_FIRSTLINE; /* Reset the bufferevent callbacks */ bufferevent_setcb(evcon->bufev, @@ -4062,6 +4055,8 @@ evhttp_get_request_connection( evcon->fd = fd; + bufferevent_enable(evcon->bufev, EV_READ); + bufferevent_disable(evcon->bufev, EV_WRITE); bufferevent_setfd(evcon->bufev, fd); return (evcon);