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

k6 gets stuck on a broken h2c connection upgrade #971

Closed
na-- opened this issue Mar 21, 2019 · 2 comments
Closed

k6 gets stuck on a broken h2c connection upgrade #971

na-- opened this issue Mar 21, 2019 · 2 comments
Assignees
Milestone

Comments

@na--
Copy link
Member

na-- commented Mar 21, 2019

If I launch the test server from this h2c example and run the following k6 script with k6 v0.24.0 (basically, emulating what curl -v --http2 http://localhost:8080/ does, even though we don't currently support h2c connections):

import http from "k6/http";

export default function () {
   http.get("http://localhost:8080/", {
       headers: {
           "Connection": "Upgrade, HTTP2-Settings",
           "Upgrade": "h2c",
           "HTTP2-Settings": "AAMAAABkAARAAAAAAAIAAAAA",
       }
   });
}

k6 will get completely stuck... And by that I mean that neither Ctrl+C will stop it, nor will k6 exit by itself when the script duration is exceeded. It will have to be killed externally... 🤕 This currently only occurs with k6 v0.24.0, but the actual root cause of the issue lies with a change introduced in Go 1.12, which we used to compile k6 v0.24.0. So other older k6 releases compiled by it, like some master Docker images (since we build them with golang:1-alpine) or user builds could also be affected.

There are at least two things, revealed by this bug, that we need to fix. The first part is that something in the event handling inside of k6 is broken, since it shouldn't be possible to get stuck like this from a single misbehaving HTTP request, whatever else may be.

The second part is that we need to properly handle these Upgrade requests in k6. @mstoykov found the root cause of the bug (by bisecting the Go source! 🥇 ) in this Go commit: golang/go@5416204. We definitely don't want to have infinite requests, at the very least, the configured timeout values should be respected. And we also should investigate other Upgrade uses like websockets. I think this should also be unit-testable as well, so we don't relapse in the future...

@cuonglm
Copy link
Contributor

cuonglm commented Aug 14, 2019

The first part is that something in the event handling inside of k6 is broken, since it shouldn't be possible to get stuck like this from a single misbehaving HTTP request, whatever else may be.

Ctrl + C won't stop k6 from running.

The current behavior of k6 is catching SIGINT and SIGTERM, trigger the engine context to cancel, then it waits the engine finishing.

IIUC, we want to catch those signals and make the engine shutdown cleanly. IOW, it does mean we completely want to ignore those signals. So the behavior is expected.

You can kill the process by sending other signal than SIGINT or SIGTERM, example, SIGQUIT, by using Ctrl + \.

@na-- na-- added this to the v0.26.0 milestone Aug 27, 2019
@na--
Copy link
Member Author

na-- commented Sep 12, 2019

This particular issue isn't about how k6 handles Ctrl+C, that's a completely separate topic we'll likely investigate in the wake of #1007 (see one of the TODOs in the PR description).

The issue here is that even when the main context is closed, regardless of how that's done (Ctrl+C, end of test, timeout, etc.), these requests are stuck and don't abort, thus causing k6 itself to also get stuck, which should never happen.

@imiric imiric self-assigned this Sep 12, 2019
imiric pushed a commit that referenced this issue Sep 23, 2019
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 pushed a commit that referenced this issue Sep 24, 2019
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 pushed a commit that referenced this issue Sep 26, 2019
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 imiric closed this as completed in 12bd929 Sep 27, 2019
cuonglm pushed a commit that referenced this issue Oct 15, 2019
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants