Hi,
Sorry for the delay. Here's a test case. I'm not sure if you'll like my
use of the limited broadcast address for simulating an ENETUNREACH error
with a TCP connection, but it's the best that I could think of. Basically,
we want to trigger a non-EINPROGRESS error in evutil_socket_connect()
immediately at the connect() in order to bring about the assertion in the
evhttp_connection_fail() error handling code.
Kevin
***** begin patch *****
diff --git a/test/regress_http.c b/test/regress_http.c
index 95511ae..9c49cae 100644
--- a/test/regress_http.c
+++ b/test/regress_http.c
@@ -3008,6 +3008,65 @@ http_stream_in_cancel_test(void *arg)
}
static void
+http_connection_fail_done(struct evhttp_request *req, void *arg)
+{
+ /* An ENETUNREACH error results in an unrecoverable
+ * evhttp_connection error (see evhttp_connection_fail()). The
+ * connection will be reset, and the user will be notified with a
NULL
+ * req parameter. */
+ tt_assert(!req);
+
+ test_ok = 1;
+
+ end:
+ event_base_loopexit(arg, NULL);
+}
+
+/* Test unrecoverable evhttp_connection errors by generating an ENETUNREACH
+ * error on connection. */
+static void
+http_connection_fail_test(void *arg)
+{
+ struct basic_test_data *data = arg;
+ ev_uint16_t port = 0;
+ struct evhttp_connection *evcon = NULL;
+ struct evhttp_request *req = NULL;
+
+ exit_base = data->base;
+ test_ok = 0;
+
+ /* auto detect a port */
+ http = http_setup(&port, data->base);
+ evhttp_free(http);
+ http = NULL;
+
+ /* Pick an unroutable address. The limited broadcast address should
do
+ * when working with TCP. */
+ evcon = evhttp_connection_base_new(data->base, NULL,
"255.255.255.255",
+ tt_assert(evcon);
+
+ /*
+ * At this point, we want to schedule an HTTP GET request
+ * server using our make request method.
+ */
+
+ req = evhttp_request_new(http_connection_fail_done, data->base);
+ tt_assert(req);
+
+ if (evhttp_make_request(evcon, req, EVHTTP_REQ_GET, "/") == -1) {
+ tt_abort_msg("Couldn't make request");
+ }
+
+ event_base_dispatch(data->base);
+
+ tt_int_op(test_ok, ==, 1);
+
+ end:
+ if (evcon)
+ evhttp_connection_free(evcon);
+}
+
+static void
http_connection_retry_done(struct evhttp_request *req, void *arg)
{
tt_assert(req);
@@ -3546,6 +3605,7 @@ struct testcase_t http_testcases[] = {
HTTP(stream_in),
HTTP(stream_in_cancel),
+ HTTP(connection_fail),
HTTP(connection_retry),
HTTP(data_length_constraints),
***** end patch *****
On Fri, May 20, 2011 at 11:20 PM, Nick Mathewson <ni...@freehaven.net>wrote:
> On Tue, May 10, 2011 at 2:46 AM, Kevin Ko <kevin.s...@gmail.com> wrote:
> > Hi,
> >
> > Patch in question:
> > -
> Fix the case when failed evhttp_make_request() leaved request in the queue.
> > -
> http://levent.git.sourceforge.net/git/gitweb.cgi?p=levent/libevent;a=commit;h=0d6622e
> >
> > The above patch introduces a failing assertion in
> > evhttp_connection_fail(). This happens because the patch defers the
> > assignment of the outstanding request to the evcon->requests list,
> > while evhttp_connection_fail() assumes that the request lies in the
> > list.
> >
> > One scenario in which this can happen is when the request list is
> > empty and a connection is made to an unreachable host. The assertion
> > will then fail after bufferevent_socket_connect() errors out (with
> > ENETUNREACH in my case).
> >
> > At the end of this message, I've attached a gdb backtrace of an
> > instance with the failed assertion and a patch that should revise the
> > original solution to have an effect only when
> > evhttp_connection_connect() fails.
>
> I ... think this is right. I'm going to check it in, but I want to
> echo Cliff's request for a test case to exercise the behavior here and
> make sure that it's actually doing the right thing.
> ***********************************************************************
> To unsubscribe, send an e-mail to majord...@freehaven.net with
> unsubscribe libevent-users in the body.
>
