More informative error message if no protocol found for http2 #305
Conversation
|
@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. |
|
@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. |
|
@Lukasa I've added a test, though I had to mock out all of |
| "No suitable protocol found, check your OpenSSL version" | ||
| in | ||
| str(exc_info) | ||
| ) |
There was a problem hiding this comment.
You can pull this assertion out of the context manager, which you will in fact need to do to get it to run because the AssertionError that gets thrown terminates the execution of this block.
| str(exc_info) | ||
| ) | ||
|
|
||
| self.tear_down() |
There was a problem hiding this comment.
Newline at the end of the file please! =)
|
Looks like https://github.com/ojii/hyper/blob/bdcf38e403c1f669d8d66d760ad16ee4c7153451/test/test_integration.py#L38breaks the other tests. Of course with my extra check, this fails now with the explicit check against Not sure what the best way to proceed is. |
|
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 |
| PROTOCOLS = hyper.http20.connection.H2_NPN_PROTOCOLS + ['', None] | ||
|
|
||
|
|
||
| # Cover our bases because NPN doesn't yet work on all our test platforms. |
Really sorry about how messy this pull request got. Should I re-open another PR with the changes as one commit? |
|
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 |
Added a more informative error message if a HTTP2 connection was unable to select a protocol, added a test for this error message. Changed integration tests to use mock to override acceptable protocols, so as not to leak those changes to other tests.
ojii
force-pushed
the
more-helpful-error-message
branch
from
ade560c to
5e133b6
Compare
|
I hope it's fine now. Let me know if there's any other changes that are needed. |
|
Looks good to me. Good work @ojii! ✨ |
|
Thank you for your great work bringing HTTP2 to Python. |
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
AssertionErrorwith 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.