search

python-hyper / hyper

HTTP/2 for Python.
http://hyper.rtfd.org/en/latest/
MIT License
1.05k stars 192 forks source link

More informative error message if no protocol found for http2 #305

Closed ojii closed 7 years ago

ojii commented 7 years ago

Today I ran into an issue with hyper, where the OpenSSL version in my environment was too old (1.0.1) so it did not have ALPN, which was required by the server. This is of course not a bug/problem with hyper itself, however the error message I got from hyper was not that helpful (it was an AssertionError with no help message). This pull request suggests a new check after wrapping the socket in SSL to see if a protocol was selected and if not tells the user that none was selected and suggests the user check their OpenSSL version.

Lukasa commented 7 years ago

@ojii Thanks for this patch, this looks like a good change. Do you mind adding a test to confirm that the message is present? That way we won't accidentally regress it.

ojii commented 7 years ago

@Lukasa will try. Had issues running the test suite (related to my network setup, not the test suite itself from what I can see), which is why there's no test included for now.

ojii commented 7 years ago

@Lukasa I've added a test, though I had to mock out all of wrap_socket. I hope this is okay.

ojii commented 7 years ago

Looks like https://github.com/ojii/hyper/blob/bdcf38e403c1f669d8d66d760ad16ee4c7153451/test/test_integration.py#L38breaks the other tests. proto becomes None in wrap_socket, but the next assertion that proto is in H2_NPN_PROTOCOLS passes because None is allowed.

Of course with my extra check, this fails now with the explicit check against None.

Not sure what the best way to proceed is.

Lukasa commented 7 years ago

So, is there any reason not to extend the assertion below rather than add a new one? If not, you should assert that the protocol is in H2_NPN_PROTOCOLS.

ojii commented 7 years ago

Looks like a comment got moved.

Really sorry about how messy this pull request got. Should I re-open another PR with the changes as one commit?

Lukasa commented 7 years ago

There's no need: if you rebase the branch to squash the commits down and then force push to the same branch, GitHub will work it out. =D

ojii commented 7 years ago

I hope it's fine now. Let me know if there's any other changes that are needed.

Lukasa commented 7 years ago

Looks good to me. Good work @ojii! :sparkles:

ojii commented 7 years ago

Thank you for your great work bringing HTTP2 to Python.