refactor: make resourceScope and Resource.use inline

[tKe]

I've opted for the allocateResourceScope approach to limit the API exposure via @PublishedAPI to a minimum

[tKe]

side note - seems like arrow.fx.coroutines.RaceNJvmTest#firstRacerOutOf2AlwaysWinsOnASingleThread is not stable

[serras]

I'm not very sure about this change for a couple of reasons:

• The implementation of resourceScope is quite big, and it's going to be copied as the method is inline.
• I'm not sure about the use case of the block not being suspend. If your resources do not use suspend, one should use arrow-autoclose instead.

[tKe]

I'm inclined to agree on the first point - however it's not a huge amount worse than the inline implementations of guaranteeCase and bracketCase.

I've actually missed the purpose of making resourceScope inline in this PR - which was to make Resource<A>.use inline.

I'll look into refactoring the finalize handling into internal methods to slim down the inlined portion.

Regarding the block not being suspend, I removed the modifier as it's purportedly redundant on a function-type parameter of an inline suspend function. I can add the modifier back in and suppress the warning as with the other similar cases.

[serras]

I've been thinking more and more about this, and I'm increasingly convinced that we may get problems out of this. My reasoning is that we want use to be inline to allow for non-local return (and break and continue from 2.1.0 on). However, are we sure that the implementation is safe in those cases?

[kyay10]

Finally blocks are triggered upon non-local return:

fun main() {
    somethingWithFinally {
        return@main
    }
}

inline fun somethingWithFinally(block: () -> Unit) {
    try {
        block()
    } finally {
        println("finally")
    }
}

so I think we're fine

[tKe]

Indeed, that was the purpose of moving the ExitCase.Completed finalizing into the finally block (and adding non-local return as a test case).

I was going to raise a separate PR to attend to the fact the guaranteeCase and bracketCase et al don't currently handle this correctly despite being inline.

I've also noticed when looking into this last night that the Resource.allocated approach as documented can lead to the finalizer being called twice in the event the finalizer throws, nor is the contents of the finalizer invoked within a NonCancellable context.

[serras]

I was going to raise a separate PR to attend to the fact the guaranteeCase and bracketCase et al don't currently handle this correctly despite being inline.

Do you mind doing this in the same PR? I think it's easier to understand the changes to this behavior in a single view than in smaller PRs.

[serras]

@kyay10 I'm worried of weirder cases like non-local return in one of the allocation/release functions, for example.

@tKe this PR changes the actual implementation of the function, not just mark it inline, so I'm trying to understand here the reason behind the bigger change.

[tKe]

The only signficant change to the actual implementation of resourceScope is the shift of the ExitCode.Completed cancelAll call into the finally clause to ensure it's always invoked even in the event of a non-local return/break/continue (as @kyay10 demonstrated).

In order to reduce the amount of code that is inlined or exposed in the API snapshots (and therefore requires consistency between versions) I opted to introduce the allocateResourceScope method which returns the ResourceScope and it's cancelAll method without exposing further implementation details.

[kyay10]

@serras a non-local return can't happen in a release function because they're stored in a list (and hence are never inline). Allocation functions currently are never inline either, but even if they were, they'd have exactly the same treatment as the whole resourceScope block itself, and hence they'd still be safely closed. I'm pretty sure that finally is guaranteed to run, unless one loops forever or does suspendCoroutine<Nothing> { }, but those cases are unavoidable. Arguably, if finally is not enough, then that'd be a bug in Kotlin itself (consider for instance that Java's finally definitely deals with early-returns. In a mental model for Kotlin's inline functions, non-local returns translate down to jumps or early-returns, but evidently there's an expectation (and likely something in the spec) that says that finally blocks are triggered when that happens)

[serras]

Superseded by #3518