HTTP_CONNECT_TIMEOUT is used as a write timeout even when timers are cleared or set to zero about libevent

[nmathewson]

Are you sure it's HTTP_CONNECT_TIMEOUT or and not HTTP_READ_TIMEOUT and HTTP_WRITE_TIMEOUT?

Looking at the code, it appears to do things like:

        if (!evutil_timerisset(&evcon->timeout)) {
                const struct timeval read_tv = { HTTP_READ_TIMEOUT, 0 };
                const struct timeval write_tv = { HTTP_WRITE_TIMEOUT, 0 };
                bufferevent_set_timeouts(evcon->bufev, &read_tv, &write_tv);
        } else {
                bufferevent_set_timeouts(evcon->bufev, &evcon->timeout, &evcon->timeout);
        }

So it would appear that clearing the timers or setting them to zero won't work. As a workaround, you can set them to a large time, like a day or two, but of course that isn't the optimal way for it to work.

In retrospect, having set_timeout(NULL) should have meant "no timeout" rather than "default timeout" -- but changing that now would break existing code by changing the documented behavior of a public API, and I'd like to avoid doing that whenever possible.

There should really be an API named something like evhttp_connection_disable_timeout and evhttp_disable_timeout to disable the timeouts for http objects.

[jrwren]

I see the code you pasted in evhttp_conection_set_timeout_tv

The code to which I am referring is in evhttp_connection_connect_
if (!evutil_timerisset(&evcon->timeout)) {
const struct timeval conn_tv = { HTTP_CONNECT_TIMEOUT, 0 };
bufferevent_set_timeouts(evcon->bufev, NULL, &conn_tv);
} else {
bufferevent_set_timeouts(evcon->bufev, NULL, &evcon->timeout);
}

So, I call evhttp_connection_set_timeout(evcon, 0) which does call evhttp_conection_set_timeout_tv as you pasted, but then on evhttp_make_request, evhttp_connection_connect_ is called and the code above is executed.

I think a fix may be to reset the timers after connection, so simply call bufferevent_set_timeouts(evcon->bufev, &evcon->timeout, &evcon->timeout); after the call to bufferevent_socket_connect_hostname. Is that reasonable or is this considered changing the document behavior of the public API? IMO this restores the behavior to what I read is documented in the api.

UPDATE:
nevermind. Now I see that this already happens in evhttp_connection_cb, but I think it is too late since the bufferevents are already added, with a timeout via event_add

UPDATE2:
Easy fix, set a huge timeout instead of timeout zero. I'm rolling with 86400. A daily reset seems reasonable.

I'd be happy to implement evhttp_connection_disable_timeout if you think it should be done. Would you recommend using evhttp_connection flags for this?
#define EVHTTP_CON_DISABLE_TIMEOUT 0x0008
and check the flag in the appropriate place?

[nmathewson]

That patch there won't actually do the right thing when evcon->timeout is 0; I believe it needs to do the same "if timeout is 0 { } else { }" thing as the other instances.

I think there should be a function which sets the timeout to "no timeout" so that that function can be mixed with functions that set the timeout again. If it uses a flag internally, that would be fine.

The code that checks the flag, checks the timeout value, and adjusts the bufferevent timeouts accordingly should probably turn into its own function, to avoid duplicated code.

Thanks!