Doesn’t work with h2c on nghttp2.org

[vfaronov]

Not sure if the problem is in Hyper/h2 or in nghttp2, but AssertionError would suggest the former:

$ hyper GET http://nghttp2.org/
Traceback (most recent call last):
  File "/home/vasiliy/tmp/env4/bin/hyper", line 11, in <module>
    load_entry_point('hyper==0.8.0.dev0', 'console_scripts', 'hyper')()
  File "/home/vasiliy/tmp/env4/lib/python3.5/site-packages/hyper/cli.py", line 259, in main
    data = request(args)
  File "/home/vasiliy/tmp/env4/lib/python3.5/site-packages/hyper/cli.py", line 249, in request
    response = conn.get_response()
  File "/home/vasiliy/tmp/env4/lib/python3.5/site-packages/hyper/common/connection.py", line 130, in get_response
    return self._conn.get_response(*args, **kwargs)
  File "/home/vasiliy/tmp/env4/lib/python3.5/site-packages/hyper/http11/connection.py", line 225, in get_response
    self
  File "/home/vasiliy/tmp/env4/lib/python3.5/site-packages/hyper/http11/response.py", line 69, in __init__
    assert self._expect_close or self._length is not None or self._chunked
AssertionError

With tcpdump, I see the HTTP/1.1 upgrade dance, and I see nghttp2.org sending the HTML. I can attach the dump if you wish.

This is with Git master versions of Hyper and h2.

[Lukasa]

Yup, that's a simple bug: we don't properly do a case-insensitive compare of the Connection header.

This would be a fairly easy bug to fix: are you interested in fixing it? The comparison is on line 216 of hyper/http11/connection.py

[vfaronov]

I would gladly contribute a fix, but I think that line should be removed altogether. Connection: upgrade does not mean “connection is being upgraded”, it means “the Upgrade header must not be forwarded by an intermediary” (see RFC 7230 § 6.7 and § 6.1), and that has no bearing on Hyper. A combination of status code and Upgrade: h2c is enough to proceed.

[Lukasa]

You missed the relevant section of RFC 7230 section 6.7:

When Upgrade is sent, the sender MUST also send a Connection header field that contains an "upgrade" connection option

We are validating this for correctness. I'm quite happy to keep the check in.

[vfaronov]

You’re putting that out of context :)

When Upgrade is sent, the sender MUST also send a Connection header
field (Section 6.1) that contains an "upgrade" connection option, in
order to prevent Upgrade from being accidentally forwarded by
intermediaries that might not implement the listed protocols.

(emphasis mine)

Connection: upgrade is a requirement for the sender of 101, not for the receiver. Imagine a buggy server that for some reason forgets to send Connection at all (but otherwise behaves correctly) — are you sure you want to go against the robustness principle and fail to work with such a server?

However, if you are sure, I’m OK with fixing the comparison as well.

[Lukasa]

Imagine a buggy server that for some reason forgets to send Connection at all (but otherwise behaves correctly) — are you sure you want to go against the robustness principle and fail to work with such a server?

Yes. Emphatically yes. Overwhelmingly yes.

A scenario in which the Connection header is not present is one in which either the server or intermediary did not adequately implement RFC 7230. If one of those middle devices did not properly implement RFC 7230 then the odds of them properly implementing RFC 7540 and RFC 7541 are approximately zero. This means that they'll run directly into our much stricter HTTP/2 stack and fall over.

I am in complete agreement with Martin Thompson: Postel was wrong. The willingness of implementations to be lenient in the face of incorrectness is one of the reasons that HTTP/1.1 has required so much re-specifying: implementations failed to follow the RFC, and so the RFC stopped reflecting the protocol as actually deployed and used.

So yes, I'm happy to fail to interop with a server that misses out this MUST.

[vfaronov]

I see your point. Will send a PR.

[vfaronov]

@Lukasa Thanks!