fix(httpext): return error on unsupported 101 response #1172
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1172 +/- ##
==========================================
+ Coverage 73.63% 73.65% +0.02%
==========================================
Files 145 145
Lines 10593 10596 +3
==========================================
+ Hits 7800 7805 +5
+ Misses 2334 2332 -2
Partials 459 459
Continue to review full report at Codecov.
|
| w.Header().Add("Connection", "Upgrade") | ||
| w.Header().Add("Upgrade", "h2c") | ||
| w.WriteHeader(http.StatusSwitchingProtocols) | ||
| })) |
There was a problem hiding this comment.
I'm not using HTTPMultiBin here to avoid a circular reference.
Sidenote: I know creating test servers at runtime like this is fairly inexpensive and often encouraged in Go testing, but this feels like a needlessly heavy end-to-end test, when a simple unit test would've been enough if MakeRequest worked with interfaces instead of concrete http APIs. Tools like moq can help with generating test structs/mocks. Let me know if this can be avoided, and your thoughts on the interface approach.
There was a problem hiding this comment.
I'm not using HTTPMultiBin here to avoid a circular reference.
This should be fixed. As we will need it at some point or another
In this particular part above you are basically doing what httpmultibin is doing ... just in a smaller scale. While I have no problems in the current situation and as a quick solution for the fact httpmultibin is using netext(if I remember correctly), for something that is probably not needed - I am fine with it. And if this would be the only place where this is needed - it's fine. But if we add more tests here (more accurately move them from the http js module here) we would probably need to use httpmultibin either way ;).
About the sidenote: My problem with mocking is that ... it tests your assumptions .. it doesn't test whether they are true. In the particular case, here it's probably fine, but in general I would say "No". Also if you go through most of the tests using httpmultibin .. if you need to set mocks for half of them it's probably going to be more work than the httpmultibin :(
I would say that definitely there are places that we can optimize and there are use cases for mocks (database connections mainly ;)) but in general if the price is not high I prefer to use the real thing ;)
There was a problem hiding this comment.
My problem with mocking is that ... it tests your assumptions .. it doesn't test whether they are true.
Agreed. Though typically mocking would be reserved for unit tests only. We should also have extensive integration and end-to-end tests, which we currently do. I suppose this distinction of the traditional test pyramid is not that important in Go where the end-to-end approach in this case is very cheap. Still, having a clear separation of test type would be helpful in other ways. For a quick sanity check I'd rather run the full unit test suite in seconds, than a suite with tests of all type in minutes. I can run the full suite once before the last push, or wait for CI. But again, I'm not sure how important this is in Go, which has good test caching, from what I've seen.
Anyways, we can continue this discussion at another time/place. :)
You're right about fixing the circular import. I'll refactor it if it needs to be used again here.
imiric
force-pushed
the
fix/971-http-h2c-upgrade-hang
branch
from
a2e3976
to
97f1915
Compare
imiric
force-pushed
the
fix/971-http-h2c-upgrade-hang
branch
from
97f1915
to
a37ddd8
Compare
There was a problem hiding this comment.
Mostly LGTM, and I'm fine with closing the issue with this PR. As I mentioned in inline comment, fixing this "properly" shouldn't be a big deal, so we can leave it as a simple TODO in the code, to be cleaned up whenever we revisit this code again, if possible.
The hanging behavior described in #971 happens after this Golang change: golang/go@5416204 , released in 1.12. If a `101 Switching Protocols` response is received, `response.Body` will be replaced by an underlying `net.Conn`, and reading it blocks indefinitely without a protocol implementation. This is not a big problem for k6 in this case, since we don't support protocol upgrades with the current HTTP API as shown in the example, so the agreed solution was to detect it and return an error. This Golang change is currently incompatible with client timeouts[1], though this will hopefully be resolved by the time we add support for h2c (#970) or other protocols. [1]: golang/go#31391 Closes #971
imiric
force-pushed
the
fix/971-http-h2c-upgrade-hang
branch
from
a37ddd8
to
12bd929
Compare
Raise an error on a
101 Switching Protocolsresponse, since k6 doesn't support protocol upgrades in the current HTTP API. This avoids the hanging behavior described in #971.See the commit description for details (I want to avoid GH's reference link spam).