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.

Sign up for GitHub

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

Jump to bottom

AutoClose and Resource cleanup #3518

Merged

serras merged 29 commits into main from kyay10/autoclose-resource-rehaul-try2

Nov 8, 2024

Collaborator

kyay10 commented Nov 1, 2024 •

edited

Loading

This subsumes #3515. This PR can be changed to use that same approach (or if #3515 is merged, this PR can just directly use it).

I've avoided source-breaking changes here. I would've liked to make autoClose inline, but that'd be source-breaking.
I changed the required methods to implement for AutoCloseScope and ResourceScope, but does Arrow consider that part of its source compatibility? I'm assuming not.

Compiler complains about resourceScope and use not needing an explicitly suspend block anymore (because it's automatically passed-through by inline). However, I think that change can be source-breaking if someone was using a function reference (compiler isn't smart enough to understand that sadly), so I suppressed that warning (which is the approach taken in Bracket as well)

I caught an error where autoCloseScope wouldn't close if an early-return happened. That was easily solvable using finally (which I think covers every possible exit scenario? Except suspendCoroutine<Nothing> { } but there's no way to stop that, and it's basically like looping forever). I fixed that error for Bracket-like functions as well, and added tests for the aforementioned and for use and resourceScope

I found a logical error where allocated didn't have the same semantics as resourceScope when it came to adding suppressed errors. Now allocated is very dumb and clearly has the same semantics, but I still added a test case anyway.

The code uses some alsos but they're used whenever the original code used them. However, I'd be happy to convert them to imperative style if you wish!

Some tests for bracket composing weren't testing suppressed exceptions correctly, and fixing that revealed a problem where throwables were reused across tests, which doesn't work with the mutation of suppressedExceptions

To discuss: do we need AcquireStep still? I recall a discussion with Simon (TODO link) about where it came from (some Scala lib) and how it's likely unnecessary anymore. We can even keep it but deprecate it and remove the DSL annotation from it so its restrictions don't apply anymore

kyay10 mentioned this pull request

AutoClose and Resource rehaul/cleanup #3431

Closed

Contributor

github-actions bot commented Nov 1, 2024 •

edited

Loading

Kover Report

File	Coverage [81.82%]
arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/Bracket.kt	86.67%
arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/Resource.kt	78.72%

Total Project Coverage	46.08%

kyay10 commented

View reviewed changes

arrow-libs/core/arrow-autoclose/src/commonMain/kotlin/arrow/AutoCloseScope.kt Show resolved Hide resolved

kyay10 commented

View reviewed changes

arrow-libs/core/arrow-autoclose/src/commonMain/kotlin/arrow/AutoCloseScope.kt Show resolved Hide resolved

kyay10 commented

View reviewed changes

arrow-libs/core/arrow-autoclose/src/commonMain/kotlin/arrow/AutoCloseScope.kt Outdated

-                  return finalizers.get().asReversed().fold(error) { acc, function ->
-                    acc.add(runCatching { function.invoke(error) }.exceptionOrNull())
+                fun close(error: Throwable?) {
+                  finalizers.value.asReversed().fold(error) { acc, finalizer ->

Collaborator Author

kyay10 Nov 1, 2024

Tried to get the code closer to the style of the corresponding resource scope implementation, mainly for the sake of future readers of the code (since in essence the two are very similar)

kyay10 commented

View reviewed changes

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/Resource.kt Show resolved Hide resolved

kyay10 commented

View reviewed changes

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/Resource.kt Outdated Show resolved Hide resolved

kyay10 commented

View reviewed changes

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/Resource.kt Outdated Show resolved Hide resolved

kyay10 commented

View reviewed changes

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/Resource.kt Outdated

-                  }?.let { throw it }
-                }
-                return Pair(allocated, release)
+              public suspend fun <A> Resource<A>.allocated(): Pair<A, suspend (ExitCase) -> Unit> = with(ResourceScopeImpl()) {

Collaborator Author

kyay10 Nov 1, 2024

The previous code didn't correctly add up the suppressed exceptions into the new one for some reason (at least that reason isn't clear to me), so I changed it (and added the relevant test) to match the semantics of resourceScope

kyay10 commented

View reviewed changes

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/Resource.kt

-                    val finalizer: suspend (ExitCase) -> Unit = { ex: ExitCase -> release(a, ex) }
-                    finalizers.update(finalizer::prependTo)
-                    a
-                  }, ::identity, { a, ex ->

Collaborator Author

kyay10 Nov 1, 2024

I don't know how to describe the previous code here other than that it clearly was refactored slowly until it rotted away. Basically, because the use function passed to bracketCase is identity, ex is always Completed, and hence the code in that finalizer is a no-op.

kyay10 commented

View reviewed changes

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/Resource.kt

-                        is ExitCase.Failure -> exitCase.failure
-                      }
-                      release(a, errorOrNull)
+                suspend fun cancelAll(exitCase: ExitCase) {

Collaborator Author

kyay10 Nov 1, 2024

This one was also refactored to look more like the auto-close impl, mainly by adding that Throwable?.add extension which cleans things up nicely

kyay10 commented

View reviewed changes

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/Resource.kt Show resolved Hide resolved

kyay10 marked this pull request as ready for review

November 1, 2024 05:54

kyay10 requested review from nomisRev and serras

November 1, 2024 05:54

tKe reviewed

View reviewed changes

Contributor

tKe left a comment

I'm wary of removing NonCancellable from install - will that allow for a resource to be instantiated and cancellation to occur before a finalizer is registered, and therefore left dangling?

Also it may be worth including similar test/fixes to bracket and guarantee around the non-local return / finally in the same PR as an overhaul of all related constructs. Especially as when fixed they'll all likely follow the same approach of "acquire a scope, action the scope and the finalize exactly once on either error or completion" but potentially with different semantics on exception handling/throwing.

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/Resource.kt Show resolved Hide resolved

arrow-libs/core/arrow-autoclose/src/commonTest/kotlin/arrow/AutoCloseTest.kt Outdated Show resolved Hide resolved

arrow-libs/core/arrow-autoclose/src/jvmMain/kotlin/arrow/throwIfFatal.kt Outdated Show resolved Hide resolved

kyay10 commented

View reviewed changes

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/Resource.kt Show resolved Hide resolved

kyay10 commented

View reviewed changes

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/Bracket.kt Show resolved Hide resolved

kyay10 commented

View reviewed changes

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/Bracket.kt Show resolved Hide resolved

kyay10 commented

View reviewed changes

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/Resource.kt Outdated

+              @OptIn(DelicateCoroutinesApi::class)
+              @Suppress("REDUNDANT_INLINE_SUSPEND_FUNCTION_TYPE")
+              public suspend inline fun <A> resourceScope(action: suspend ResourceScope.() -> A): A {
+                val (scope, cancelAll) = resource { this }.allocated()

Collaborator Author

kyay10 Nov 1, 2024

A little hacky, but it results in no new ABI added. We might instead want a function allocateResourceScope(): Pair<ResourceScope, suspend (ExitCase) -> Unit>

Contributor

tKe Nov 1, 2024

I think this is fine - if anything a comment to highlight that this is the ResourceScope for clarity but it's internal and imo understandable from the context.

kyay10 commented

View reviewed changes

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/Resource.kt Show resolved Hide resolved

kyay10 requested a review from tKe

November 1, 2024 18:59

tKe approved these changes

View reviewed changes

Contributor

tKe left a comment

Not sure if my approval counts for much, but I think this is all much improved, leaner and more composed.

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/Bracket.kt Show resolved Hide resolved

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/Resource.kt Show resolved Hide resolved

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/Resource.kt Outdated

+              @OptIn(DelicateCoroutinesApi::class)
+              @Suppress("REDUNDANT_INLINE_SUSPEND_FUNCTION_TYPE")
+              public suspend inline fun <A> resourceScope(action: suspend ResourceScope.() -> A): A {
+                val (scope, cancelAll) = resource { this }.allocated()

Contributor

tKe Nov 1, 2024

I think this is fine - if anything a comment to highlight that this is the ResourceScope for clarity but it's internal and imo understandable from the context.

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/Resource.kt Show resolved Hide resolved

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/Resource.kt

-                  return install({ a }, release)
-                }
+                public suspend infix fun <A> Resource<A>.releaseCase(release: suspend (A, ExitCase) -> Unit): A =
+                  bind().also { a -> onRelease { release(a, it) } }

Contributor

tKe Nov 1, 2024

Do we need to be concerned about bind() being called in a non-cancellable context here? (It wasn't previously, either) - possibly a question for @nomisRev ?

It does mean resource { ... } release {} acts differently to install({ ... }) { _, _ -> }

Collaborator Author

kyay10 Nov 1, 2024

Resource is basically a fancy name for any function that you might call taking a ResourceScope, so I'd assume that either the function registers its own release (and this one here is extra for some reason?) or that the function registers some resources, but it leaves the final one for you to register yourself.
Inline usage of resource like that is stylistically tempting, but it feels easier to just do it imperatively.
Maybe what we actually want is for resource to be non-cancellable by default?

Member

serras commented Nov 1, 2024

@tKe @kyay10 thanks very much for this great effort. Mostly for my own sake, could you make #3515 also part of this PR? That way I can get a better picture of what's going on here.

Collaborator Author

kyay10 commented Nov 1, 2024

@serras This PR covers the features of #3515, but implements it differently, hence I didn't base it on its commits. I.e. if this is merged, #3515 is redundant, but this does more than #3515

Contributor

tKe commented Nov 1, 2024

This covers what #3515 set out to achieve, with the same sureties on finalizers being called with non-local returns, but also simplifies and shares the previously-duplicated (and subtly different) implementations of resourceScope and Resource.allocate (the verb does make so much more sense than the past-participle allocated). It also extends the same fixes for non-local returns to guaranteeCase, bracketCase and autoCloseScope.

kyay10 mentioned this pull request

Arrow Fx DSL #3443

Draft

8 tasks

kyay10 added the 2.0.0 label

Collaborator Author

kyay10 commented Nov 4, 2024 •

edited

Loading

Hey @serras, I've taken the only binary-breaking change out of this PR and into #3522. Thus, this PR can wait until some 2.x version, so you can take your time with it!
There was technically another (close in AutoCloseScope used to return Nothing? and I wanted to change it to Unit) but I just undid it. It was a minor cosmetic difference


          Simplify AutoCloseScope interface to a single method.

bd9d967

Remove unnecessary suppressions for usages of AutoClosable
Force closing on non-local returns

kyay10 added 6 commits

November 4, 2024 04:34


          Call finalizer in one place in finalizeCase

8d1dd60


          Call scope.close in one place in autoCloseScope

64d9850


          Short-circuit fatal resource release

3a5346c


          Add tests for short-circuiting fatal bracket

affbdb9


          Reduce usage of CompletableDeferred in tests

212a0fb


          Make DefaultAutoCloseScope.close binary-compatible

8131f37

kyay10 force-pushed the kyay10/autoclose-resource-rehaul-try2 branch from 7373491 to 8131f37 Compare

November 4, 2024 04:34


          Update API files

44656bc

kyay10 added 2.x.x and removed 2.0.0 labels

kyay10 mentioned this pull request

Remove suspend modifier from ResourceScope.onRelease #3522

Merged

serras requested changes

View reviewed changes

Member

serras left a comment

Thanks for the hard work on this!

Apart from a few clarifications, my main concern here is that some tests change their behavior, and we need to be 100% sure that this is OK.

arrow-libs/core/arrow-autoclose/src/commonTest/kotlin/arrow/AutoCloseTest.kt Outdated Show resolved Hide resolved

arrow-libs/core/arrow-autoclose/src/commonTest/kotlin/arrow/AutoCloseTest.kt Outdated Show resolved Hide resolved

kotlin-js-store/yarn.lock Outdated Show resolved Hide resolved

arrow-libs/fx/arrow-fx-coroutines/src/commonTest/kotlin/arrow/fx/coroutines/BracketCaseTest.kt Show resolved Hide resolved

Member

serras commented Nov 5, 2024

@kyay10 sorry, I created some conflicts by merging the other two open PRs...

Collaborator Author

kyay10 commented Nov 5, 2024

No problem with the conflicts! They should be trivial to fix up

kyay10 added 3 commits

November 5, 2024 13:39


          Delete kotlin-js-store/yarn.lock

8e8837e


          Revert "Delete kotlin-js-store/yarn.lock"

52695f4

This reverts commit 8e8837e.


          Return yarn.lock to pre-PR state

f10318e

serras reviewed

View reviewed changes

arrow-libs/core/arrow-autoclose/src/commonMain/kotlin/arrow/AutoCloseScope.kt Show resolved Hide resolved

serras reviewed

View reviewed changes

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/Bracket.kt Outdated Show resolved Hide resolved

kyay10 and others added 3 commits

November 5, 2024 14:07


          Merge branch 'main' into kyay10/autoclose-resource-rehaul-try2

# Conflicts:
#	arrow-libs/fx/arrow-fx-coroutines/api/arrow-fx-coroutines.api
#	arrow-libs/fx/arrow-fx-coroutines/api/arrow-fx-coroutines.klib.api
#	arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/Resource.kt
#	arrow-libs/fx/arrow-fx-coroutines/src/commonTest/kotlin/arrow/fx/coroutines/ResourceTest.kt


          Auto-update API files


          Inline runReleaseAndRethrow and simplify

184927b

kyay10 commented

View reviewed changes

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/Bracket.kt

+                  }
+                } catch (e: Throwable) {
+                  e.nonFatalOrThrow()
+                  when (ex) {

Collaborator Author

kyay10 Nov 5, 2024

This can be written as (ex.errorOrNull ?: throw e).addSuppressed(e) but I went for clarity instead, so that the code now reads like "if fa completed successfully, rethrow the error from its finalizer. Otherwise, suppress that error"


          Use CompletableDeferred in tests and add shouldHaveCompleted

bdda97a

kyay10 requested a review from serras

November 6, 2024 18:51


          Merge branch 'main' into kyay10/autoclose-resource-rehaul-try2

5d55fd1

serras approved these changes

View reviewed changes

serras merged commit fa2fa40 into main

11 checks passed

serras deleted the kyay10/autoclose-resource-rehaul-try2 branch

November 8, 2024 09:15

serras mentioned this pull request

refactor: make resourceScope and Resource.use inline #3515

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels