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

Do not cancel context on exception #65

Merged
merged 1 commit into from
Aug 2, 2024
Merged

Do not cancel context on exception #65

merged 1 commit into from
Aug 2, 2024

Conversation

agnivade
Copy link
Owner

@agnivade agnivade commented Aug 2, 2024

Sometimes, exception can happen if a callback runs
after the test has exited. In that case, cancelling
a context fails the test which is not intentional.

Since cancelling happens anyways after a test exits,
there is not much point manually cancelling it
and we try to keep parity with non-browser environments.

Fixes #60

Sometimes, exception can happen if a callback runs
after the test has exited. In that case, cancelling
a context fails the test which is not intentional.

Since cancelling happens anyways after a test exits,
there is not much point manually cancelling it
and we try to keep parity with non-browser environments.

Fixes #60
@agnivade
Copy link
Owner Author

agnivade commented Aug 2, 2024

@hajimehoshi @paralin - Apologies for the delay on this. Wondering if you would be able to give this a try in your CI envs and let me know if this fixes it. Thanks!

@hajimehoshi
Copy link
Contributor

I am testing this now: https://github.com/hajimehoshi/ebiten/actions/runs/10211074516

@agnivade
Copy link
Owner Author

agnivade commented Aug 2, 2024

Hmm .. it seems like this issue is fixed, but this seems to have appeared on Ubuntu now? Has this happened before? I've never seen it fail on Linux before.

@hajimehoshi
Copy link
Contributor

This failed due to "websocket url timeout reached" unfortunately 🤔

@agnivade
Copy link
Owner Author

agnivade commented Aug 2, 2024

That is what I mentioned in my comment before. This is #59 but we have only seen this in Windows before. Has this failed in Linux before? Or is this the first time?

@hajimehoshi
Copy link
Contributor

Not sure but I think this happened on Linux before

@paralin
Copy link

paralin commented Aug 2, 2024

I will test it

@hajimehoshi
Copy link
Contributor

Now the tests passed. I guess the situation gets much better than before!

@agnivade
Copy link
Owner Author

agnivade commented Aug 2, 2024

Cool. Looks like this issue is fixed. The "websocket url timeout" is a separate issue which also needs to be looked into. Thanks for testing this out Hajime san!

I will wait for @paralin's confirmation before merging this.

@paralin
Copy link

paralin commented Aug 2, 2024

I think this might still be an instance of the error: https://github.com/aperturerobotics/bifrost/actions/runs/10213182411/job/28258052721#step:14:527

That one is not a context canceled but rather "Go program has already exited" - not sure if this pr was intended to fix that or not?

@agnivade
Copy link
Owner Author

agnivade commented Aug 2, 2024

No my PR does not fix that. That error appears from the wasm_exec.js, but the real thing that actually fails the test is the cancelling of context. From your test output, it appears that the test has passed successfully.

I think it's good to keep the output to give an indication that some callback has run after the test has finished, to give an opportunity for the developer to fix the issue. But it does the fail the test.

@agnivade agnivade merged commit 6e5bbb8 into master Aug 2, 2024
12 checks passed
@paralin
Copy link

paralin commented Aug 2, 2024

@agnivade Ah, alright. Well I would prefer to not have that error, because it is not something I can fix - it's not really possible to ensure no callback will happen after the program exits.

@agnivade agnivade deleted the cancelCtx branch August 2, 2024 15:23
@agnivade
Copy link
Owner Author

agnivade commented Aug 2, 2024

The error comes from here. Arguably it could be removed, but IMO it's there as an indicator to the user that something unexpected has happened. As you said, sometimes it might not really be possible, but sometimes it might be. Atleast in those cases, the user should know that there's something to be fixed.

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.

Tests flake when a callback is called after the test finishes
3 participants