AutoClose and Resource rehaul/cleanup #3431
Conversation
Kover Report
|
There was a problem hiding this comment.
@kyay10 amazing work like always
Okay, so you made the "single abstract method" installing the onClose, or onRelease finaliser. That indeed affects the AcquireStep in resource, as we've discussed on Slack, but this gives more flexibility.
So both these types, give us the ability to install a non-suspending finaliser, and a suspending finaliser. I think that's a fair DSL, although quite different from where we started. This is great, because in a lot of these cases I've kind-of gotten tunnel vision after so many years.
I'd like to wait the feedback of others.
There was a problem hiding this comment.
Requesting some changes, or want to discuss some changes:
- Quite some source compatible breaking changes, which introduces complexity for migration. I would prefer to avoid this, unless we have a strong reason to do so.
- A lot of unrelated code was changed from bodies to expression, introducing a lot of scoped functions (also). I am personally not a fan of those, especially not of using also within a function body. Can we keep the changes smaller? I 100% support improving readability of operations for users. but I would prefer to keep them separate from actual changes.
Thank you again for the great work @kyay10 👏 👏 Excited by these changes!
| @PublishedApi | ||
| internal actual fun Throwable.throwIfFatal(): Throwable = | ||
| when(this) { | ||
| is VirtualMachineError, is ThreadDeath, is InterruptedException, is LinkageError, is CancellationException -> throw this | ||
| is VirtualMachineError, is ThreadDeath, is InterruptedException, is LinkageError -> throw this |
There was a problem hiding this comment.
Hate that this function cannot be private. I really do not want to expose it 😓
This is why I had the CancellationException explicitly in the try/catch, because I rather had a correct definition here, than one specific to AutoCloseable...
| public data class Cancelled(val exception: CancellationException) : ExitCase() | ||
| public data class Failure(val failure: Throwable) : ExitCase() | ||
| public sealed interface ExitCase { | ||
| public data object Completed : ExitCase |
| public val ExitCase.throwableOrNull: Throwable? get() = when (this) { | ||
| ExitCase.Completed -> null | ||
| is ExitCase.Cancelled -> exception | ||
| is ExitCase.Failure -> failure | ||
| } | ||
|
|
There was a problem hiding this comment.
Can we define this inside ExitCase so that we don't need an import for it? Preferably as a function I'd say, that'd be more in line with the APIs we now from Result.
I also think exceptionOrNull is a more common name, than throwableOrNull.
| return a | ||
| cancelAll(ExitCase(e)) | ||
| error("Unreachable, cancelAll should throw the exception passed to it. Please report this bug to the Arrow team.") | ||
| }.also { cancelAll(Completed) } |
There was a problem hiding this comment.
Can we remove also here? I'd prefer regular function invocation after } if possible.
I try to avoid scoping function everywhere where they might be redundant. I also see you changed a lot of function bodies, to expressions, and rely on also instead. I personally feel this really degrades readability.
This is a common occurrence in this PR, I rather not have these unrelated code changes purely for style reasons. It introduces a lot of code churn for no strong reason. Also takes away the focus of this PR.
| * It results either in [ExitCase.Completed], [ExitCase.Cancelled] or [ExitCase.Failure] depending on the terminal state of [Resource] lambda. | ||
| */ | ||
| @ResourceDSL | ||
| public suspend inline fun <A> ResourceScope.install( |
There was a problem hiding this comment.
Why are these being defined as extension functions, this breaks source compatibility with 1.x.x. Why not define this in the interface directly?
| @PublishedApi | ||
| internal class DefaultAutoCloseScope : AutoCloseScope { | ||
| private val finalizers = Atomic(emptyList<(Throwable?) -> Unit>()) | ||
| public inline fun <A> AutoCloseScope.autoClose( |
There was a problem hiding this comment.
Idem, this introduces a source breaking change, to support suspend in the acquire step. If that is needed, I think users should prefer onClose, or Resource instead.
So I would prefer to revert this also.
|
Thanks for your work on making the DSL better, @kyay10. I really like everything which removes constraints from our DSLs, so if we can have Having said so, we promised to keep source compatibility between 1.2.x and 2.0 (except for the changes in Optics KSP), and some of these changes break that source compatibility. I would really like to keep this promise: we've already broken code in the past, and I really want to make things right this time. This means that, at the very least, the same functions need to be exposed from the same package or class they originally were. Going also in the direction pointed by @nomisRev, maybe this could be easier to review if the fundamental changes were broken from the refactoring changes. In general in Arrow we try to not refactor things like |
This PR simplifies the implementations of
AutoCloseScopeandResourceScopeand allows some of their functions to beinline. It also makesonReleasenot suspending.