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

Return timeout error when appropriate for setup/teardown fix #869 #890

Merged
merged 3 commits into from Jan 7, 2019

Conversation

Projects
None yet
3 participants
@MStoykov
Copy link
Contributor

MStoykov commented Jan 4, 2019

Previously because of the way js interrupt works it was that the js vm
will not get interrupt signal for some time after the timeout has
happened. This would be especially true in cases of lower (1) cpu counts
or under heavy load.
Because calls from inside js to go code will return when the
context is Done we will probably still end setup/teardown functions in
approximately timeout time but it might not actually be marked as
timeouted.

To fix this after calls to js functions we check that the timeout has
been passed if setted. If this has happened we return a new specific
timeout error that can later be used to distinguish that timeout has
happened. There is also some additional code to catch that an actual
interrupt has happened and that it is because of timeout.

Return timeout error when appropriate for setup/teardown fix #869
Previously because of the way js interrupt works it was that the js vm
will not get interrupt signal for some time after the timeout has
happened. This would be especially true in cases of lower (1) cpu counts
or under heavy load.
Because calls from inside js to go code will return when the
context is *Done* we will probably still end setup/teardown functions in
approximately timeout time but it might not actually be marked as
timeouted.

To fix this after calls to js functions we check that the timeout has
been passed if setted. If this has happened we return a new specific
timeout error that can later be used to distinguish that timeout has
happened. There is also some additional code to catch that an actual
interrupt has happened and that it is because of timeout.

@MStoykov MStoykov requested a review from na-- Jan 4, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 4, 2019

Codecov Report

Merging #890 into master will decrease coverage by 0.07%.
The diff coverage is 9.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #890      +/-   ##
==========================================
- Coverage   70.01%   69.93%   -0.08%     
==========================================
  Files         111      112       +1     
  Lines        8754     8764      +10     
==========================================
  Hits         6129     6129              
- Misses       2229     2238       +9     
- Partials      396      397       +1
Impacted Files Coverage Δ
lib/timeout_error.go 0% <0%> (ø)
js/runner.go 85.95% <20%> (-1.45%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d219b19...fb82cbf. Read the comment docs.

@na--

na-- approved these changes Jan 4, 2019

Copy link
Member

na-- left a comment

Mostly LGTM, only not sure about the necessity of that cancel()... My gut feeling is that it might be needed and it won't hurt to stay, so we should probably leave it as it was 😄 In any case, I don't have the time to check it in depth now.

@@ -297,7 +296,16 @@ func (r *Runner) runPart(ctx context.Context, out chan<- stats.SampleContainer,
}

v, _, err := vu.runFn(ctx, group, fn, vu.Runtime.ToValue(arg))
cancel()

This comment has been minimized.

Copy link
@na--

na-- Jan 4, 2019

Member

I am not sure this cancel() is without a reason. Are you sure that there isn't anything that might be left dangling if we remove it? After all, maybe the script execution will finish normally, without cancelling the context, but something will still be left listening to it? If you need access to the original ctx below, just rename it?

cancel()

// deadline is reached so we have timeouted but this might've not been registered correctly
if deadline, ok := ctx.Deadline(); ok && time.Now().After(deadline) {

This comment has been minimized.

Copy link
@na--

na-- Jan 4, 2019

Member

Super cool solution! 😄

@MStoykov MStoykov merged commit 7266aed into master Jan 7, 2019

6 checks passed

ci/circleci: build-docker-images Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: test-go110 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@MStoykov MStoykov deleted the fixSetupTeardownTimeout branch Jan 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.