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: use await instead of waitFor in async tests #2690

Merged
merged 8 commits into from
May 10, 2024

Conversation

gabrielmer
Copy link
Contributor

Description

In async functions, we should use await instead of waitFor. The usage of waitFor in async tests can generate crashes.

Changes

  • change use of waitFor for await in async tests

Copy link

github-actions bot commented May 9, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2690-rln-v1

Built from 2ee8958

Copy link

github-actions bot commented May 9, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2690-rln-v2

Built from 2ee8958

Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks! 💯

Copy link
Contributor

@SionoiS SionoiS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!

@chaitanyaprem
Copy link
Contributor

Interesting, What is the reason for crash? Is there some bug in waitFor macro or the way use it?

Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds very interesting, would be good understand the reason! Thanks!

@gabrielmer
Copy link
Contributor Author

So in principle,waitFor should not be used in async procedures as it blocks the current thread and it defeats the purpose of async procedures.

Even on its implementation there's a comment stating not to be called from an async proc

https://github.com/status-im/nim-chronos/blob/8a306763cec8105fa83574b56734b0f66823f844/chronos/internal/asyncfutures.nim#L628-L638

Regarding the crash we were having, I haven't yet figured out what exactly was going on that caused it. There were other tests with waitFor being used inside async procedures that did not crash.

In our case, we figured out that the crash happened when trying to write to global variables right after the waitFor, but not sure yet why exactly that's a problem.

@gabrielmer gabrielmer force-pushed the fix-use-of-waitfor-in-async-tests branch from 127c384 to 907b4d6 Compare May 10, 2024 09:13
@gabrielmer gabrielmer merged commit a37c9ba into master May 10, 2024
14 of 15 checks passed
@gabrielmer gabrielmer deleted the fix-use-of-waitfor-in-async-tests branch May 10, 2024 12:13
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.

6 participants