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

Request context is passed to PushIfNotCancelled instead of VU context #1260

Closed
cuonglm opened this issue Dec 4, 2019 · 0 comments · Fixed by #1261
Closed

Request context is passed to PushIfNotCancelled instead of VU context #1260

cuonglm opened this issue Dec 4, 2019 · 0 comments · Fixed by #1261
Assignees
Labels
Milestone

Comments

@cuonglm
Copy link
Contributor

cuonglm commented Dec 4, 2019

This script fails with current go master:

import http from "k6/http";
import { check } from "k6";
import { Counter } from "k6/metrics";

let statusCheck = { "status is 200": (r) => r.status === 200 }
let myCounter = new Counter("setup_teardown");

export let options = {
	iterations: 5,
	thresholds: {
		"setup_teardown": ["count == 2"],
		"iterations": ["count == 5"],
		"http_reqs": ["count == 7"],
	},
};

export function setup() {
	check(http.get("http://test.loadimpact.com"), statusCheck) && myCounter.add(1);
};

export default function () {
	check(http.get("http://test.loadimpact.com"), statusCheck);
};

export function teardown() {
	check(http.get("http://test.loadimpact.com"), statusCheck) && myCounter.add(1);
};
$ go run main.go run ./t.js

          /\      |‾‾|  /‾‾/  /‾/
     /\  /  \     |  |_/  /  / /
    /  \/    \    |      |  /  ‾‾\
   /          \   |  |‾\  \ | (_) |
  / __________ \  |__|  \__\ \___/ .io

  execution: local
     output: -
     script: ./t.js

    duration: -,  iterations: 5
         vus: 1, max: 1

    done [==========================================================] 5 / 5

    ✓ status is 200

    checks.....................: 100.00% ✓ 7   ✗ 0
    data_received..............: 9.7 kB  14 kB/s
    data_sent..................: 623 B   913 B/s
    http_req_blocked...........: avg=134.9ms  min=110.1ms  med=134.9ms  max=159.7ms  p(90)=154.74ms p(95)=157.22ms
    http_req_connecting........: avg=110.37ms min=110.05ms med=110.37ms max=110.7ms  p(90)=110.63ms p(95)=110.66ms
    http_req_duration..........: avg=112.33ms min=112.09ms med=112.33ms max=112.56ms p(90)=112.51ms p(95)=112.54ms
    http_req_receiving.........: avg=133µs    min=98µs     med=133µs    max=168µs    p(90)=161µs    p(95)=164.5µs
    http_req_sending...........: avg=62.5µs   min=60µs     med=62.5µs   max=65µs     p(90)=64.5µs   p(95)=64.75µs
    http_req_tls_handshaking...: avg=0s       min=0s       med=0s       max=0s       p(90)=0s       p(95)=0s
    http_req_waiting...........: avg=112.13ms min=111.93ms med=112.13ms max=112.33ms p(90)=112.29ms p(95)=112.31ms
  ✗ http_reqs..................: 2       2.931485/s
    iteration_duration.........: avg=168.24ms min=113.77ms med=114.05ms max=272.5ms  p(90)=245.25ms p(95)=258.87ms
  ✓ iterations.................: 5       7.328713/s
  ✓ setup_teardown.............: 2       2.931485/s
    vus........................: 1       min=1 max=1
    vus_max....................: 1       min=1 max=1

There are two problems for current code base:

  • Client.Timeout is set and intended to pass to request context, but it doesn't due to the bug in Go standard library. If we set the request context timeout explicitly, it leads to the second problem.
  • Currently, the request context is passed to PushIfNotCancelled, which is completely wrong, since when we want to use VU context instead.
@cuonglm cuonglm added the bug label Dec 4, 2019
@cuonglm cuonglm self-assigned this Dec 4, 2019
cuonglm added a commit that referenced this issue Dec 4, 2019
Go 1.13 and below has a bug that client timeout will not be propagated
to http request context. That bug was fixed already and will be present
in go1.14 (golang/go#31657).

With current go master, our TestSetupTeardownThresholds fails. The
reason is that we only push sample if the request context not done. When
the bug in Go stdlib was not fixed, our own http Client.Timeout is not
passed to request context, so when client makes request, it does not
aware about the request context, and always push the sample.

Now when Client.Timeout is passed to request context, as long as the
client finish the request, the request context is consider done, and we
do not push the sample anymore.

To fix it, simply wrapping the request context with the client timeout.

Fixed #1260
cuonglm added a commit that referenced this issue Dec 5, 2019
Go 1.13 and below has a bug that client timeout will not be propagated
to http request context. That bug was fixed already and will be present
in go1.14 (golang/go#31657).

With current go master, our TestSetupTeardownThresholds fails. The
reason is that we only push sample if the request context not done. When
the bug in Go stdlib was not fixed, our own http Client.Timeout is not
passed to request context, so when client makes request, it does not
aware about the request context, and always push the sample.

Now when Client.Timeout is passed to request context, as long as the
client finish the request, the request context is consider done, and we
do not push the sample anymore.

To fix it, simply wrapping the request context with the client timeout.

Fixed #1260
@cuonglm cuonglm changed the title Client.Timeout is not passed to request context Request context is passed to PushIfNotCancelled instead of VU context Dec 5, 2019
cuonglm added a commit that referenced this issue Dec 6, 2019
Go 1.13 and below has a bug that client timeout will not be propagated
to http request context. That bug was fixed already and will be present
in go1.14 (golang/go#31657).

Currently, we set Timeout in our Client, but do not use it else where.
It will not affect the request context for now, but does when go1.14
release.

To make it compatible and works correctly with other go versions, we set
the request context explicitly with the timeout.

Update #1260
cuonglm added a commit that referenced this issue Dec 6, 2019
PushIfNotDone is intended to use with VU context instead of request
context. This is a bug in current code base, but it is not triggered due
to the bug in Go net/http. A request context can be in cancelled state
due to be cancelled or timeouted. PushIfNotDone won't see the difference
and will skip pushing metrics for timeouted requests.

Fixing it by passing VU context to PushIfNotDone instead of request ctx.

Fixes #1260
cuonglm added a commit that referenced this issue Dec 6, 2019
PushIfNotDone is intended to use with VU context instead of request
context. This is a bug in current code base, but it is not triggered due
to the bug in Go net/http. A request context can be in cancelled state
due to be cancelled or timeouted. PushIfNotDone won't see the difference
and will skip pushing metrics for timeouted requests.

Fixing it by passing VU context to PushIfNotDone instead of request ctx.

Fixes #1260
@cuonglm cuonglm added this to the v0.27.0 milestone Dec 6, 2019
cuonglm added a commit that referenced this issue Dec 17, 2019
Go 1.13 and below has a bug that client timeout will not be propagated
to http request context. That bug was fixed already and will be present
in go1.14 (golang/go#31657).

Currently, we set Timeout in our Client, but do not use it else where.
It will not affect the request context for now, but does when go1.14
release.

To make it compatible and works correctly with other go versions, we set
the request context explicitly with the timeout.

Update #1260
cuonglm added a commit that referenced this issue Dec 17, 2019
PushIfNotDone is intended to use with VU context instead of request
context. This is a bug in current code base, but it is not triggered due
to the bug in Go net/http. A request context can be in cancelled state
due to be cancelled or timeouted. PushIfNotDone won't see the difference
and will skip pushing metrics for timeouted requests.

Fixing it by passing VU context to PushIfNotDone instead of request ctx.

Fixes #1260
cuonglm added a commit that referenced this issue Dec 17, 2019
PushIfNotDone is intended to use with VU context instead of request
context. This is a bug in current code base, but it is not triggered due
to the bug in Go net/http. A request context can be in cancelled state
due to be cancelled or timeouted. PushIfNotDone won't see the difference
and will skip pushing metrics for timeouted requests.

Fixing it by passing VU context to PushIfNotDone instead of request ctx.

Fixes #1260
cuonglm added a commit that referenced this issue Dec 17, 2019
Go 1.13 and below has a bug that client timeout will not be propagated
to http request context. That bug was fixed already and will be present
in go1.14 (golang/go#31657).

Currently, we set Timeout in our Client, but do not use it else where.
It will not affect the request context for now, but does when go1.14
release.

To make it compatible and works correctly with other go versions, we set
the request context explicitly with the timeout.

Update #1260
cuonglm added a commit that referenced this issue Dec 17, 2019
PushIfNotDone is intended to use with VU context instead of request
context. This is a bug in current code base, but it is not triggered due
to the bug in Go net/http. A request context can be in cancelled state
due to be cancelled or timeouted. PushIfNotDone won't see the difference
and will skip pushing metrics for timeouted requests.

Fixing it by passing VU context to PushIfNotDone instead of request ctx.

Fixes #1260
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant