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

Sleep pauses entire execution including websocket intervals #1070

Open
nicodn opened this issue Mar 8, 2023 · 4 comments
Open

Sleep pauses entire execution including websocket intervals #1070

nicodn opened this issue Mar 8, 2023 · 4 comments
Labels
Area: OSS Content Improvements or additions to community/oss documentation Priority: High

Comments

@nicodn
Copy link

nicodn commented Mar 8, 2023

Brief summary

Using sleep() with durations longer than 30 seconds cause our websocket sessions to get disconnected by the server because ping is not firing despite it being in an interval

k6 version

k6 v0.42.0

OS

macOS

Docker version and image (if applicable)

No response

Steps to reproduce the problem

I'm using the experimental support for Websockets & in an setInterval calling socket.ping()

If I add sleep(45) after connecting the websocket, when the sleep ends the server has already disconnected the socket due to lack of pings.

Expected behaviour

Ideally intervals & timers & timeouts should continue despite calls to sleep(), or perhaps a different type of sleep might need to be supported?

Actual behaviour

The ping calls don't get attempted until after sleep() ends. Not sure what the legacy socket behaviour is...

@nicodn nicodn changed the title Sleep pauses entire execution websocket intervals Sleep pauses entire execution including websocket intervals Mar 8, 2023
@na--
Copy link
Member

na-- commented Mar 9, 2023

The old k6 sleep() function is incompatible with JS event loops, it blocks the whole event loop... 😞 It was fine when k6 didn't have an event loop in every VU and everything was synchronous, but now it can be a big problem, as you have seen. This is why other JS runtimes don't have a blocking sleep() function, they have setTimeout() or something like that.

I will move this issue to the docs repository, since we probably need to adjust the documentation and discourage users from using sleep(), given that we have more and more async functions in the k6 JS APIs... cc @mstoykov

perhaps a different type of sleep might need to be supported?

It is, k6 also has setTimeout() and setInterval(), see https://k6.io/docs/javascript-api/k6-experimental/timers/

@na-- na-- transferred this issue from grafana/k6 Mar 9, 2023
@na-- na-- added Area: OSS Content Improvements or additions to community/oss documentation Priority: High labels Mar 9, 2023
@mstoykov
Copy link
Contributor

mstoykov commented Mar 9, 2023

I do wonder though ... what to do?

I do think it is too early to outright be "don't use any syncrhnouns calls - use only async ones". There are both IMO too many sharp corners and not enough support.

Even http.asyncRequest is not enough for http request as anyone wanting to use any of the "html" methods to start request suddenly goes into synchrous http requests calls :(. Fixing that is likely not going to be easy.

I also don't really want to go to every API and be "This is syncrhouns call - do not use with asynchrnous calls if possible." and the reverse :(.

Having a general explanation around synchrouns and asyncrhouns calls and the event loop, seems like the best call - but the again it needs to be linked from everywhere.

So I would prefer if we first figure out what the plan is before somebody spends days writing and editing a ton of pages.

@nicodn
Copy link
Author

nicodn commented Mar 9, 2023

Thanks for the awesomely rapid feedback guys! Reading your comments helped me grasp the change I had to make and now I'm using either a sleep call to simulate users closing the app and timers to pause user actions while the wss continues in the background

@MattDodsonEnglish
Copy link
Contributor

Do I understand that we should now advise against all sleep calls in favor of some async method? @mstoykov @na--

That's a pretty big change docs-wise. Will require changing a lot of scripts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: OSS Content Improvements or additions to community/oss documentation Priority: High
Projects
None yet
Development

No branches or pull requests

4 participants