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

Fix panic with setInterval in k6/ws #2896

Merged
merged 1 commit into from
Feb 2, 2023
Merged

Fix panic with setInterval in k6/ws #2896

merged 1 commit into from
Feb 2, 2023

Conversation

mstoykov
Copy link
Contributor

@mstoykov mstoykov commented Feb 2, 2023

The panic happens when interval between 0 and 1 is given.

The code that tested whether the interval was > 0 correctly converted to
floats first and then back to time. And then checked it in order to
prevent this very panic.

Unfortunately it then did the same calculation again but this time by
converting the interval directly to int before multiplying by
milliseconds. Which ends up rounding it to 0. Making the whole result
zero and leading to time.NewTicker to panic.

This was already done correctly for setTimeout when the same panic was
found there and the wrong conversation was added for setInterval.

Previous commit df3ad1b

The panic happens when interval between 0 and 1 is given.

The code that tested whether the interval was > 0 correctly converted to
floats first and then back to time. And then checked it in order to
prevent this very panic.

Unfortunately it then did the same calculation again but this time by
converting the interval directly to int before multiplying by
milliseconds. Which ends up rounding it to 0. Making the whole result
zero and leading to time.NewTicker to panic.

This was already done correctly for setTimeout when the same panic was
found there and the wrong conversation was added for setInterval.

Previous commit df3ad1b
@mstoykov mstoykov added this to the v0.43.0 milestone Feb 2, 2023
@github-actions github-actions bot requested review from na-- and olegbespalov February 2, 2023 13:43
@codecov-commenter
Copy link

codecov-commenter commented Feb 2, 2023

Codecov Report

Merging #2896 (8484c98) into master (4e88cc1) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

❗ Current head 8484c98 differs from pull request most recent head a444065. Consider uploading reports for the commit a444065 to get more accurate results

@@            Coverage Diff             @@
##           master    #2896      +/-   ##
==========================================
- Coverage   76.93%   76.87%   -0.06%     
==========================================
  Files         225      223       -2     
  Lines       16867    16862       -5     
==========================================
- Hits        12976    12963      -13     
- Misses       3060     3065       +5     
- Partials      831      834       +3     
Flag Coverage Δ
ubuntu 76.87% <100.00%> (+0.02%) ⬆️
windows ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
js/modules/k6/ws/ws.go 86.08% <100.00%> (ø)
loader/filesystems.go 60.00% <0.00%> (-40.00%) ⬇️
js/common/initenv.go 66.66% <0.00%> (-33.34%) ⬇️
api/v1/metric.go 77.27% <0.00%> (-4.55%) ⬇️
cmd/tests/test_state.go 87.75% <0.00%> (-4.09%) ⬇️
metrics/metric_type.go 78.94% <0.00%> (-3.51%) ⬇️
lib/netext/httpext/error_codes.go 94.52% <0.00%> (-2.74%) ⬇️
lib/netext/httpext/error_codes_syscall_windows.go
cmd/ui_windows.go
lib/executor/vu_handle.go 95.32% <0.00%> (+0.93%) ⬆️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mstoykov mstoykov requested a review from codebien February 2, 2023 14:09
@mstoykov mstoykov merged commit b85d09d into master Feb 2, 2023
@mstoykov mstoykov deleted the fixWSPanic branch February 2, 2023 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants