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

Connection upgrade fails when request body is not empty #160

Closed
noteflakes opened this issue Aug 3, 2021 · 1 comment · Fixed by #171
Closed

Connection upgrade fails when request body is not empty #160

noteflakes opened this issue Aug 3, 2021 · 1 comment · Fixed by #171

Comments

@noteflakes
Copy link

noteflakes commented Aug 3, 2021

Hi and thanks for this wonderful gem! When upgrading an HTTP/1.1 connection (e.g. using curl --http2), if the HTTP/1.1 request body passed to HTTP2::Server#upgrade is not empty, the upgrade will fail with the following traceback:

Traceback (most recent call last):
        12: from /home/sharon/repo/tipi/lib/tipi.rb:39:in `block (2 levels) in accept_loop'
<snip>
         3: from /home/sharon/.gem/gems/http-2-0.11.0/lib/http/2/server.rb:102:in `upgrade'
         2: from /home/sharon/.gem/gems/http-2-0.11.0/lib/http/2/stream.rb:102:in `receive'
         1: from /home/sharon/.gem/gems/http-2-0.11.0/lib/http/2/stream.rb:321:in `transition'
/home/sharon/.gem/gems/http-2-0.11.0/lib/http/2/stream.rb:604:in `end_stream?': undefined method `include?' for nil:NilClass (NoMethodError)

The fix is trivial:

class HTTP2::Stream
  def end_stream?(frame)
    case frame[:type]
    when :data, :headers, :continuation
      frame[:flags]&.include?(:end_stream)
    else false
    end
  end
end

The fix is just to add the safe navigation operator when checking for frame[:flags], since it's its not set when the body is not empty.

@mullermp
Copy link
Collaborator

@noteflakes Did you still need this patch? I've got a PR to finally add this. I didn't use safe navigation so that the ? method returns a boolean (though nil is falsey).

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 a pull request may close this issue.

2 participants