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

Remove suspend modifier from ResourceScope.onRelease #3522

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

kyay10
Copy link
Collaborator

@kyay10 kyay10 commented Nov 4, 2024

This allows it to be called in non-suspending contexts like runInterruptible which is important for resource safety when interacting with interruptible Java APIs. It also just doesn't need to be suspend, and in fact, it can be the SAM that defines ResourceScope, although that's not implemented here.
This is the only binary breaking change needed for #3518.
I'd really like to get this approved for 2.0, since otherwise we'd have to delay it to 3.0 :(

@kyay10 kyay10 added the 2.0.0 Tickets / PRs belonging to Arrow 2.0 label Nov 4, 2024
Copy link
Contributor

github-actions bot commented Nov 4, 2024

Kover Report

File Coverage [70.24%]
arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/Resource.kt 70.24%
Total Project Coverage 46.07%

@serras
Copy link
Member

serras commented Nov 4, 2024

Note that the current architecture is that anything which does not collaborate with coroutines (including Java interop) should go to arrow-autoclose, and Resource should build on top of that to include integration with coroutines. There are some cases where the compiler suggests dropping suspend, but we still want to keep it to signal that this is only for coroutines.

By the way, is this something that should also be in AutoClose? The overall idea is to align the two APIs as much as possible, except suspend and other coroutine-related elements.

@kyay10
Copy link
Collaborator Author

kyay10 commented Nov 4, 2024

Oh sorry let me clarify: this is Java interop with coroutines. runInterruptible is a kotlinx.coroutines function that interops with Java, but it's just one example of a use case where you want to call onRelease without being able to suspend. onRelease is specific to Resource because it has to support suspending release handlers.
AutoCloseScope I think doesn't currently have an onRelease analog. I'm adding one though in the aforementioned PR under the name onClose. I don't think adding onClose in this PR is important though, since it's a trivial addition and doesn't break any compatibilities.

@serras
Copy link
Member

serras commented Nov 5, 2024

Ah, I misunderstood where the suspend is being removed (I thought it was from the argument). If in the new implementation this becomes possible, I would go for it.

@serras serras merged commit f0f4e74 into main Nov 5, 2024
11 checks passed
@serras serras deleted the kyay10/non-suspend-onRelease branch November 5, 2024 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0.0 Tickets / PRs belonging to Arrow 2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants