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

Ensure teardown is completed when observation coroutines are canceled #699

Merged
merged 3 commits into from
Jun 27, 2024

Conversation

mmeisel
Copy link
Contributor

@mmeisel mmeisel commented Jun 20, 2024

It turns out that any suspending function called after cancelation of the coroutine will itself be canceled. The kotlin documentation recommends using withContext(NonCancellable) {} in these cases.

Fixes #677

I wanted to write a test for this, but the observation tests don't use any suspend functions so it wasn't clear to me how. It did fix the problem for me, however.

@mmeisel mmeisel requested review from twyatt and a team as code owners June 20, 2024 19:09
@mmeisel mmeisel requested a review from Phoenix7351 June 20, 2024 19:09
@twyatt twyatt added the patch Changes that should bump the PATCH version number label Jun 21, 2024
@twyatt

This comment was marked as outdated.

Copy link
Member

@twyatt twyatt left a comment

Choose a reason for hiding this comment

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

I did not test it, but I understand why this should fix the issue. 👍
Only minor ask I have is to update the code comment slightly.

core/src/commonMain/kotlin/Observers.kt Outdated Show resolved Hide resolved
@twyatt
Copy link
Member

twyatt commented Jun 25, 2024

@mmeisel I plan on testing this in the next day or two. If everything works as expected then I'll merge and cut a release. Thanks again for the PR!

@twyatt twyatt merged commit ee0182c into JuulLabs:main Jun 27, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Changes that should bump the PATCH version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot reobserve a characteristic after canceling a job containing a call to observe
3 participants