Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raise error on unexpected EOF. #163

Closed
wants to merge 1 commit into from

Conversation

mickm
Copy link
Contributor

@mickm mickm commented Nov 3, 2014

Fixes infinite read() loop in the case where the server closes the
connection without sending a valid HTTP response.

Fixes infinite read() loop in the case where the server closes the
connection without sending a valid HTTP response.
@mickm
Copy link
Contributor Author

mickm commented Nov 4, 2014

Hi, thanks for the great library! This is a patch for a snag I ran into today, the commit message has the details. There is a spec, but you can reproduce the problem manually like so:

nc -l 2345 &
ruby -rhttp -e 'puts HTTP.get("http://localhost:2345/").to_s' &
kill %1
# ruby process is now in infinite loop w/ 100% cpu

Feedback appreciated!

@tarcieri
Copy link
Member

tarcieri commented Nov 4, 2014

I'm a bit uncertain about the semantic implications of this... I'd be curious what @ixti thinks.

Looks like the tests pass aside from some Rubbocrap.

@ixti
Copy link
Member

ixti commented Nov 4, 2014

It's actually a kind of small regression. Return of read_more supposed to be handled and stop partial reading once we reached closed socket. But at the current state it's really broken.

Now I see that this is the cause of most cases described in #152
I'm gonna merge and re-work your fix. EOFError should be handled in readpartial and finish response when occur. It should not be bubbled up (see #46).

@tarcieri i'm more curious when you switched from "I'm totally fine if it enforces consistent style" to "rubbocrap" ;))

@sferik
Copy link
Contributor

sferik commented Nov 4, 2014

@tarcieri wrote:

Looks like the tests pass aside from some Rubbocrap.

@ixti wrote:

@tarcieri i'm more curious when you switched from "I'm totally fine if it enforces consistent style" to "rubbocrap" ;))

If you look at the RuboCop failures, they’re actually related to quality metrics, not style.

spec/support/example_server.rb:6:3: C: Cyclomatic complexity for do_GET is too high. [9/8]
  def do_GET(request, response) # rubocop:disable MethodName
  ^^^
spec/support/example_server.rb:6:3: C: Method has too many lines. [24/22]
  def do_GET(request, response) # rubocop:disable MethodName
  ^^^

If we want to relax our standards for number of lines per method and cyclomatic complexity in order to merge this patch, we can do that, but ideally, we would could create a patch that doesn’t increasing the code complexity above our current threshold.

@ixti
Copy link
Member

ixti commented Nov 4, 2014

@sferik sorry, I thought that ;)) smile was enough to omit <sarcasm> tag :D

I have different style preferences compared to yours (the main difference is single quotes versus double quotes), but other than that I would like to keep current rubocop config as strict as possible. example_server.rb really looks a bit difficult to read at the moment, but for now I'm gonna relax some bits just for that file, merge and improve the fix and then will provide a small specs/example_server refactoring... Or will do that at once... :D

@mickm I don't want to merge this patch as is - there are couple of issues one of them I explained above. The other is specs. They actually cover only one particular case: when connection was unpredictably closed before headers were received. So I'm gonna extend specs and add EOF handling to readpartial - it's OK if headers phase will fail with EOFError, as in this case we can't even potentially have a valid response, while EOFError might be legit in some cases when headers were received.

@ixti ixti self-assigned this Nov 4, 2014
@mickm
Copy link
Contributor Author

mickm commented Nov 5, 2014

That all makes sense. I'm happy to take a shot at any of those changes, although it sounds like you're already on it :) Let me know if I can help out in any way and thanks for the feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants