Menu
▾
▴
#116 rate-limiting breaks http/data_length_constraints on Windows
It seems that my rate-limiting patch breaks the http/data_length_constraints unit test on Windows. Looking though the code that changed, I find that I can make the test pass again if I replace the "howmuch=readmax;" line in bufferevent_readcb() in bufferevent.c with a no-op. I have no real idea why; the effect of removing that line is only to make sure evbuffer_read() always gets called with -1 rather than the current default maximum 16384.
IIRC, the code is now failing in WSARecv with a "connection reset" error, though I couldn't swear to that.
I have no clue what's going on here.
yea its the ECONNRESET
calling evbuffer_read() with -1 doesn't make it pass for me for some reason.
the test works for me as of:
commit 165d30e31a167215c03a9104423f2767c9ff9c9a
Author: Nick Mathewson <nickm@torproject.org>
Date: Wed Dec 30 14:29:56 2009 -0500
Fix compilation of rate-limiting code on win32.
The problem seems timing related. when I set a break point on evbuffer_read, the test fails
Yea, when Sleep(100); is added to the top of evbuffer_read() the test fails too
Hm. From the information we have, I can't really tell whether this is a unit test bug or a bufferevent but or an http bug. Any thoughts?
Interestingly, this happens for me now with MSVC2008, but not with mingw-gcc.
add shutdown before close
The issue seems to be that the server responds with an error and closes the connection before reading all of the client's request. Since there is data still in the socket''s buffer, a RST is sent to the client, and the client's connection fails with ECONNRESET before it has a chance to read the response. Adding shutdown(SHUT_WR) before closing the connection seems to fix the test for me.
So the question now becomes, do we consider this a bug or a missing feature? And do we do a general fix (in bufferevent somewhere), or an http only fix?
Didn't you mention something about implementing bufferevent_flush to call shutdown when the user passes BEV_FINISHED? IIRC, this would wait till 2.1.
I did, and having that wait till 2.1 is fine.
But my question is more of "is this something that nearly everybody using bufferevents is going to want to have happen every time they close a connection?" If so, then bufferevent_socket's current behavior is kinda broken.
I'm not sure if it's a good idea or not to have an implicit shutdown when CLOSE_ON_FREE is set, although it probably wouldn't hurt anything. evhttp in its current form wouldn't take advantage because it closes the socket manually.
Okay; merged your patch here, and added https://sourceforge.net/tracker/?func=detail&aid=3081573&group_id=50884&atid=461325 to track the "we should do shutdowns" issue. Thanks!