Discussion:
Possible bug in http.c
(too old to reply)
Ronen Mizrahi
2014-02-26 15:29:18 UTC
Permalink
Raw Message
Hi,

I have come across what seems to be a bug in http implementation, where an
http server does not detect closed connections until the next time it
attempts to write something.

After some investigation I found the issue in evhttp_write_buffer() from
http.c. There is a comment there that says:
"We don't disable reading, since we *do* want to learn about any close
events"

However reading is already disabled so it needs to be enabled for close
events to be sent. Once I enabled reading the above problem was solved and
the http server was able to properly detect connections closed by the
client and send a close event.

I am new to libevent, happy to send a patch if desirable but first wanted
to run it by someone.

Best,

Ronen Mizrahi
Nick Mathewson
2014-03-01 19:09:02 UTC
Permalink
Raw Message
Post by Ronen Mizrahi
Hi,
I have come across what seems to be a bug in http implementation, where an
http server does not detect closed connections until the next time it
attempts to write something.
After some investigation I found the issue in evhttp_write_buffer() from
"We don't disable reading, since we *do* want to learn about any close
events"
However reading is already disabled so it needs to be enabled for close
events to be sent. Once I enabled reading the above problem was solved and
the http server was able to properly detect connections closed by the client
and send a close event.
I am new to libevent, happy to send a patch if desirable but first wanted to
run it by someone.
Yeah, if that's the behavior, that does indeed sound wrong. We should
learn about close events where the other side closes the socket.

I'd be happy to take a patch for that; the important thing for a such
a patch IMO would be that the patch should be tested. (For example,
what should happen if, instead of closing, the other side sends more
data, or some HTTP protocol stuff, or something? Whatever the right
answer is, we need to make sure that the code really does that.)

Please let me know if there's any more I can do to help get a patch in here.

best wishes,
--
Nick
***********************************************************************
To unsubscribe, send an e-mail to ***@freehaven.net with
unsubscribe libevent-users in the body.
Ronen Mizrahi
2014-03-03 20:06:32 UTC
Permalink
Raw Message
Thanks Nick. We have been testing it for the last few days and it seems to
work well. What do you mean in terms of testing? Is there any automated set
of tests that need to be made for proper verification?
Post by Ronen Mizrahi
Post by Ronen Mizrahi
Hi,
I have come across what seems to be a bug in http implementation, where
an
Post by Ronen Mizrahi
http server does not detect closed connections until the next time it
attempts to write something.
After some investigation I found the issue in evhttp_write_buffer() from
"We don't disable reading, since we *do* want to learn about any close
events"
However reading is already disabled so it needs to be enabled for close
events to be sent. Once I enabled reading the above problem was solved
and
Post by Ronen Mizrahi
the http server was able to properly detect connections closed by the
client
Post by Ronen Mizrahi
and send a close event.
I am new to libevent, happy to send a patch if desirable but first
wanted to
Post by Ronen Mizrahi
run it by someone.
Yeah, if that's the behavior, that does indeed sound wrong. We should
learn about close events where the other side closes the socket.
I'd be happy to take a patch for that; the important thing for a such
a patch IMO would be that the patch should be tested. (For example,
what should happen if, instead of closing, the other side sends more
data, or some HTTP protocol stuff, or something? Whatever the right
answer is, we need to make sure that the code really does that.)
Please let me know if there's any more I can do to help get a patch in here.
best wishes,
--
Nick
***********************************************************************
unsubscribe libevent-users in the body.
Nick Mathewson
2014-03-03 20:30:57 UTC
Permalink
Raw Message
Post by Ronen Mizrahi
Thanks Nick. We have been testing it for the last few days and it seems to
work well. What do you mean in terms of testing? Is there any automated set
of tests that need to be made for proper verification?
It should pass the unit/regression tests. (They run with "make
check".) It would be good if they were include a check for the
specific case that this patch fixes, to make sure that the patch
really fixes it.

The current evhttp tests are in test/regress_http.c , but any kind of
little test program that fits into the testing infrastructure would be
fine with me. (Don't take too many style cues from the contents of
test/regress_http.c -- many of the tests there follow a legacy testing
style.)

best wishes,
--
Nick
***********************************************************************
To unsubscribe, send an e-mail to ***@freehaven.net with
unsubscribe libevent-users in the body.
Ronen Mizrahi
2014-03-05 02:43:27 UTC
Permalink
Raw Message
It looks like the patch does create issues with subsequent HTTP requests
made on the same connection (for persistent connections) and therefore we
won't submit it.

The original issue reported however is still very much in effect. It is
particularly problematic for long-polling type requests where the server
uses evhttp_send_reply_start() and then every once in a while does
evhttp_send_reply_chunk(). If the client closes the connection, the server
won't detect it during the period between different chunks but rather only
when attempting to send the next chunk. This period can be several minutes
so this can be a real issue for servers that handle many connections.
Post by Nick Mathewson
Post by Ronen Mizrahi
Thanks Nick. We have been testing it for the last few days and it seems
to
Post by Ronen Mizrahi
work well. What do you mean in terms of testing? Is there any automated
set
Post by Ronen Mizrahi
of tests that need to be made for proper verification?
It should pass the unit/regression tests. (They run with "make
check".) It would be good if they were include a check for the
specific case that this patch fixes, to make sure that the patch
really fixes it.
The current evhttp tests are in test/regress_http.c , but any kind of
little test program that fits into the testing infrastructure would be
fine with me. (Don't take too many style cues from the contents of
test/regress_http.c -- many of the tests there follow a legacy testing
style.)
best wishes,
--
Nick
***********************************************************************
unsubscribe libevent-users in the body.
Nick Mathewson
2014-03-05 16:01:54 UTC
Permalink
Raw Message
Post by Ronen Mizrahi
It looks like the patch does create issues with subsequent HTTP requests
made on the same connection (for persistent connections) and therefore we
won't submit it.
The original issue reported however is still very much in effect. It is
particularly problematic for long-polling type requests where the server
uses evhttp_send_reply_start() and then every once in a while does
evhttp_send_reply_chunk(). If the client closes the connection, the server
won't detect it during the period between different chunks but rather only
when attempting to send the next chunk. This period can be several minutes
so this can be a real issue for servers that handle many connections.
What kind of issues did the original patch cause? Maybe we can track
those issues down and fix it?

peace,
--
Nick
***********************************************************************
To unsubscribe, send an e-mail to ***@freehaven.net with
unsubscribe libevent-users in the body.
Ronen Mizrahi
2014-03-05 20:34:57 UTC
Permalink
Raw Message
It looks like I was mistaken and the patch was not the cause of the issue I
mentioned. I have continued testing it and it seems to perform correctly so
I am attaching it.

Note that I chose to enable read when write is performed, instead of just
eliminating all cases where read is disabled, because we did not know what
side-effects that might have. In contrast, enabling read while writing,
suggests that the worst side-effects one may experience will be related to
HTTP pipelining (the only case where the client is expected to write
something while waiting for response from server), and so I briefly tested
it as well (via telnet) and all seems to work as expected. It probably
makes sense to test http pipelining some more though.

I did not get a chance to create another unit test, however.

Best,

Ronen
Post by Ronen Mizrahi
Post by Ronen Mizrahi
It looks like the patch does create issues with subsequent HTTP requests
made on the same connection (for persistent connections) and therefore we
won't submit it.
The original issue reported however is still very much in effect. It is
particularly problematic for long-polling type requests where the server
uses evhttp_send_reply_start() and then every once in a while does
evhttp_send_reply_chunk(). If the client closes the connection, the
server
Post by Ronen Mizrahi
won't detect it during the period between different chunks but rather
only
Post by Ronen Mizrahi
when attempting to send the next chunk. This period can be several
minutes
Post by Ronen Mizrahi
so this can be a real issue for servers that handle many connections.
What kind of issues did the original patch cause? Maybe we can track
those issues down and fix it?
peace,
--
Nick
***********************************************************************
unsubscribe libevent-users in the body.
Loading...