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

Allow http_parser to decide if there is still body to read #424

Closed
wants to merge 1 commit into from

Conversation

Bonias
Copy link
Contributor

@Bonias Bonias commented Aug 3, 2017

It mostly fixes problem when connection which uses "Transfer-Encoding: chunked" header is closed to early, before the terminating chunk is received. In such case "http" returned partial body instead of raising error.


Here is how the problem can be reproduced:

Run this app (rackup -p 3000 -E none -s puma):

$ cat config.ru 
class App
  def call(env)
    [200, {}, self]
  end

  def each
    yield 'one'
    sleep 1
    yield 'two'
    sleep 2
    yield 'three'
    sleep 3
    yield 'four'
    sleep 4
    yield 'five'
    sleep 5
    yield 'six'
  end
end

use Rack::Chunked
run App.new

Use irb console to make http request to the app and then kill the app (in the middle of the request):

2.2.3 :001 > require 'http'
 => true 
2.2.3 :002 > HTTP.get('http://localhost:3000/').body.to_s
 => "onetwo" 

All tests are passing so my fix seems to be fine, but if you think otherwise let me know and I'll try change it.

It mostly fixes problem when connection which uses "Transfer-Encoding: chunked"
header is closed to early before the terminating chunk is received. In such
case "http" returned partial body instead of raising error.
@Bonias Bonias force-pushed the fix-unfinished-body-handling branch from b4cac7d to e101a4d Compare August 3, 2017 12:58
@ixti ixti self-assigned this Aug 3, 2017
@woutdegeyter
Copy link

@ixti I am wondering if you would happen to have any news on this PR?
We have encountered multiple issues due to this already

@ixti
Copy link
Member

ixti commented Aug 18, 2017

Thanks! Merged as 72723df

@ixti ixti closed this Aug 18, 2017
@Bonias
Copy link
Contributor Author

Bonias commented Sep 4, 2017

@ixti Sorry to bother you, is there any chance for release a new version in the near future?

@ixti
Copy link
Member

ixti commented Sep 5, 2017

@Bonias thanks for reminder :D I will handle that during this week (or over this weekend).

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.

3 participants