Why was `ResourceScope` marked as subclass opt-in ...

# arrow-contributors

[CLOVIS]

Why was ResourceScope marked as subclass opt-in required?

[phldavies]

https://github.com/arrow-kt/arrow/issues/3811

[CLOVIS]

@Youssef Shoaib [MOD]

[CLOVIS]

I would argue "we communicate clearly that inheriting from them is inadvisable" hasn't been achieved, at least there should be a comment on it explaining why I think

[CLOVIS]

Why is it inadvisable? It seems to me that it's impossible to integrate Resources into any existing DSL without implementing ResourceScope

[Youssef Shoaib [MOD]]

Well, for one, you can make a ResourceScope whose onRelease is just a no-op. That's very inadvisable to me.
In general, it feels like you need to "know what you're doing" to implement ResourceScope and ensure that its contracts are followed properly (each block is called on release, in LIFO order, exactly once, with proper exception handling).

at least there should be a comment on it explaining why

100%. I'll make a PR or something to update the KDoc.

It seems to me that it's impossible to integrate Resources into any existing DSL without implementing ResourceScope

Are contexts not enough here? You can expose ResourceScope by having it as a context in your DSL. You can also store it around in your custom DSL objects if you need access to it internally. I'm not sure that I see a huge need to implement ResourceScope on your own.

[Youssef Shoaib [MOD]]

Oh also, it "might break across releases" in case we want to maybe change the methods that are necessary to override in ResourceScope. Tbh, that's a weaker argument now since there's just the one method, namely onRelease, but previously, we got bit by that.

[simon.vergauwen]

To add my 2 cents. I didn't add the annotation but it feels the same as CoroutineScope which official advice is to also never extend?

I.e. doing class MyService(..): CoroutineScope or class MyDSL(...): CoroutineScope is advised against. Composition should be preferred.

So I agree that context(ResourceScope) or exposing a val scope: ResourceScope is probably safer. Also yes, 2.4 is around the corner. Time to all go harder on context parameters 😬

[CLOVIS]

For context, my use-case is the integration of Resources and the Prepared test framework, so that resource clean up happens in Prepared-track fixtures. Contexts don't help me, because I do need a custom implementation at the end.

[Youssef Shoaib [MOD]]

Well, ultimately, you can just opt-in if you're in the edge-case where it's really important for you to inherit from it! That's a very valid use case you have there.
Please do lmk if I am wrong here! I mostly suggested that subclass opt in because kotlinx-coroutines do it (to dissuade implementation since it can be error-prone), and we may benefit from wanting to change up the required methods to override in the future.
Btw, you might instead want to use the allocate method (I think that's what it's called) which lets you integrate with any other cleanup mechanism you might want. It is under opt-in as well, but at least it doesn't need you to keep track of cleanup order or anything like that.
From memory, you can do something like:

val (scope, onRelease) = resource { this }.allocate()
// pass scope wherever you need it
myFrameworkCleanupHandler.register { onRelease(...) }

(see also the implementation of resourceScope, which does exactly that!)
Having this as an option means that it's even less likely that you'll need to extend ResourceScope directly

[CLOVIS]

I didn't add the annotation but it feels the same as CoroutineScope which official advice is to also never extend?

CoroutineScope isn't marked with subclass opt-in. I think it's because inheritance-by-delegation is allowed and legitimate. Job is marked as opt-in though. Flow is marked subclass-opt-in because methods get added without default values.

All three of these types are very easy to extend though, they all have constructors, builders, etc. ResourceScope doesn't (AFAIK)

[CLOVIS]

Well, ultimately, you can just opt-in if you're in the edge-case where it's really important for you to inherit from it! That's a very valid use case you have there.

That's why I need a comment explaining the reasons and the guarantees 😅 "We think you don't need it" is very different from "things will break if you do"

[CLOVIS]

(I'm not against it being marked subclass-opt-in)

[Youssef Shoaib [MOD]]

I do wish there were a flag on subclass opt-in that allowed inheritance-by-delegation...

[CLOVIS]

My implementation is here: https://gitlab.com/opensavvy/groundwork/prepared/-/blob/568ced80fbf4c606e55a588875b95f3726bee7bf/compat/compat-arrow/src/commonMain/kotlin/coroutines/Resource.kt#L200

[Youssef Shoaib [MOD]]

It is a failure of communication that allocate wasn't clearly the tool to use here.
Just to check, do you think it does the trick for you here?
My thinking now is we should have a clear message saying "hey, if you're thinking of implementing this, maybe use allocate instead" and then after list all the guarantees that a ResourceScope implementation is supposed to follow (LIFO, handling errors)
allocate doesn't need you to be aware of those guarantees. All that it needs is that you call its onClose eventually, and do so exactly once.

[simon.vergauwen]

To be honest this implementation looks fine to me on first sight. I am making some assumptions of how the code works though. No cancellation?

[CLOVIS]

The assumptions I can confirm:
• cleanUp runs in LIFO order
• cleanUp runs in a finally block after the entire test

[Youssef Shoaib [MOD]]

I think it might have some issues though. How does it handle one of the release actions failing? What we do is we catch that, and pass the exception to all the other release actions (even if, originally, we finished successfully).

[CLOVIS]

Can you link the allocate KDoc? I'm on mobile, it's not easy to find

[phldavies]

I seem to recall doing something like this in the past to get a ResourceScope for manual use:

val (scope, release) = @OptIn(DelicateCoroutinesApi::class) resource { this }.allocate()

[CLOVIS]

Good point about release actions failing. However in this case I think it's more convenient if the behavior matches Prepared's fixtures behavior, which don't have this fall through behavior

[Youssef Shoaib [MOD]]

https://apidocs.arrow-kt.io/arrow-fx-coroutines/arrow.fx.coroutines/allocate.html?query=suspend%20fun%20%3CA%3E%20Resource%3CA%3E.allocate():%20Pair%3CA,%20suspend%20(ExitCase)%20-%3E%20Unit%3E
Untested implementation that uses allocate instead:

fun TestDsl.makeResourceScope(): ResourceScope {
    val (scope, release) = resource { this }.allocate()
    launch(Dispatchers.Unconfined) {
		cleanUp("Release resource on success", onFailure = false) {
			release(ExitCase.Completed)
		}

		cleanUp("Release resource on failure", onSuccess = false) {
			release(ExitCase(RuntimeException("The real exception which caused the test to fail is unknown")))
		}
	}
    return scope
}

If that's how Prepared functions, then that's very fair, but it can obviously be surprising. This subtlety of behaviour, I think, makes the opt-in a reasonable requirement

[simon.vergauwen]

it's more convenient if the behavior matches Prepared's fixtures behavior, which don't have this fall through behavior

Agreed.

[Youssef Shoaib [MOD]]

I know it's a ridiculous trick, but it's how resourceScope is implemented! That's largely because I wanted to hide away the implementation class from public API, and allocate was already there!

[phldavies]

(the 😆 is at you doing the same trick I mentioned that @simon.vergauwen responded to with 💀 )

[CLOVIS]

I don't understand your example, how does it know that the cleanup is about the resource clean up? 😅

[Youssef Shoaib [MOD]]

So release here is a function returned by allocate. Under the hood, allocate creates this resource scope that stores all the onRelease functions in a list. The release function returned encapsulates all the tricky logic to run them in proper order and do error handling.
We simply schedule release to run onCleanup, so all the onRelease actions will already be in the relevant list

[CLOVIS]

I see

[Youssef Shoaib [MOD]]

In other words, allocate lets you worry about just a singular release function that you need to schedule to run eventually, and it deals with everything else.

[CLOVIS]

The difference with my implementation, then, is that mine registers a Prepared-style finalizer for each resource, whereas this one only generates a single finalizer for the entire test, that does everything.

I don't know which is best 🤔

[Youssef Shoaib [MOD]]

It depends a lot on your semantics then. As long as you communicate that the provided ResourceScope registers Prepared-style finalizers, then it's fine I believe.

[phldavies]

prepared {
	makeResourceScope().block()
}

surely when used this register a per-prepared resourcescope and tear down that entire prepared uniformly?

[CLOVIS]

Actually I'm not sure that's a good solution. If the user creates resources and Prepared finalizers, the order won't be LIFO, because all the resources will be joined together even if there were interspersed Prepared finalizers.

[CLOVIS]

Honestly, no idea how legitimate that would be though. I certainly wouldn't write that kind of code.

[Youssef Shoaib [MOD]]

The decisions for how the normal resourceScope behaves are best suited for an application-style environment where you absolutely want every finalizer to run no matter what. I can see why tests might want different behaviour
> If the user creates resources and Prepared finalizers, the order won't be LIFO
That's a good point! In this case, either implement ResourceScope on your own, or maybe make Prepared finalizers use the same ResourceScope under the hood (although I can see that being difficult).

[CLOVIS]

Prepared doesn't depend on Arrow, so that's not an option.

[phldavies]

or have a context(ResourceScope) Prepared<T>.bind(): T that installs the prepared finalizer into the resource scope instead of the testdsl

[Youssef Shoaib [MOD]]

You can generify it. Have some registerFinalizer callback that by default uses your own implementation, but in an arrow-specific block uses ResourceScope. At this point, you might as well just accept the opt-in I think.

[CLOVIS]

@phldavies that would mean cleanUp behaves differently whether the compiler can find an instance of ResourceScope or not, which may vary even within a single test.

[Youssef Shoaib [MOD]]

Would you be more likely to add a dependency onto a library that purely had AutoCloseScope and ResourceScope? arrow-autoclose is already a thing, so I don't imagine arrow-fx-resources would be a difficult pill to swallow (and it'd be very, very tiny)

[CLOVIS]

Sadly, the name ‘arrow’ brings user reluctance 😓

[Youssef Shoaib [MOD]]

brb gonna make shadow copies of Arrow under my io.github package. Surely that brings more credibility /s

[Youssef Shoaib [MOD]]

More seriously, I can imagine that your cleanUp function ultimately stores finalizers somewhere and then iterates over them, so maybe you can abstract over that and use ResourceScope when it's available, and your own implementation otherwise.

[CLOVIS]

I've had people complain about Spine because it has an optional arrow dependency 😭

[CLOVIS]

That's why we need to continue with the propaganda

[CLOVIS]

Anyway I don't think having the behavior of finalizers change based on the presence of some other lib (no matter which) is a good idea

[phldavies]

Or worker harder on Simon/Alejandro and make arrow part of the kotlin stdlib 😉

[Youssef Shoaib [MOD]]

Maybeee something like ResourceScope could be? I think other parts are more out-there though and would never be added (Either, Raise, a lot of the fx libraries).
I wouldn't want every part of Arrow in the stdlib in the slightest. Which is funny because, at the same time, I think most projects would benefit from using Arrow one way or another. Personally, I reach for Arrow less often than I reach for a lot of the stdlib, but when I do, it's always an incredible addition

[CLOVIS]

I think parMap & co definitely should be in kotlinx coroutines, yeah

[CLOVIS]

I wish Raise was in the stdlib, it's so much better than rich errors

[phldavies]

The behaviour of the finalizers wouldn't change - I'd say within a preparedResource { } you shouldn't be able to "get" a prepared using val a by myOtherPrepared but you can bind it, which plays nicely with the different scoping - outside of preparedResource it will be lifecycle-managed by TestDsl, but inside it collaborates with ResourceScope.

[Youssef Shoaib [MOD]]

Not to steer the conversation too far away from the topic, but I think they're different things (Raise vs rich errors). Rich errors are quick and snappy for short lines of code, and for storing data around, while Raise is great for larger blocks of code. In some sense, rich errors eat into the market for Option and Either way more than they do for Raise

[Youssef Shoaib [MOD]]

Looking at your Finalizers implementation, I don't see why tweaking the behaviour slightly to recover from finalizer errors would be unexpected. After all, finalizers are expected to be ran and to clean up resources, so it could be surprising that they don't run if another finalizer fails.
If you do change the behaviour, then now the behaviour is basically identical to resourceScope, so you can freely swap between the two as the basis of your implementation. Your finalizers would then be in the right order.
Btw, your implementation could be greatly simplified. For one, you can capture the TestDsl inside the finalizer internally. Then, you don't need to store the metadata about whether the finalizer should run on success or failure, instead accessing it directly thru the TestDsl inside the block. Finally, you can println inside the block as well. This'd turn your Finalizers class into basically a ResourceScope implementation

[CLOVIS]

outside of preparedResource it will be lifecycle-managed by TestDsl, but inside it collaborates with ResourceScope.

That doesn't match the mental model of prepared {} . prepared {} is to declare fixtures that are used by the test, they are allowed to outlast their own call until the end of that specific test execution. For example, you can write

val db by prepared {
    val db = createDatabase()

    launchInBackground {
        db.keepActive()
    }

    cleanUp("db") { db.close() }

    db
}

[CLOVIS]

Not to steer the conversation too far away from the topic, but I think they're different things (Raise vs rich errors). Rich errors are quick and snappy for short lines of code, and for storing data around, while Raise is great for larger blocks of code. In some sense, rich errors eat into the market for Option and Either way more than they do for Raise

That is my understanding as well, but that is not how they are described in the KEEP nor in the blog posts, nor is it how they're viewed by the community.

[Youssef Shoaib [MOD]]

I think the community sentiment will shift once they get to actually use it and see the upsides and downsides. I think, in a world without Raise (which, sadly, a lot of people live in), rich errors are a huge upgrade, so I understand such messaging. I still see them as a big upgrade for library uses since they let you efficiently have "extra null-like values" without degrading your code to using Any? and unsafe casts, or wrapping your values in `Option`s everywhere

[Youssef Shoaib [MOD]]

(I've never had a better editing experience outside an IDE because your repos use tabs instead of spaces! Genuinely might be the best argument I've seen for them)

internal class Finalizers {
    typealias Finalizer = suspend (Boolean) -> Unit
	private val lock = Mutex()
	private val finalizers = ArrayList<Finalizer>()

	suspend fun register(block: Finalizer) {
		lock.withLock("register") { finalizers.add(block) }
	}

	suspend fun executeAllFinalizers(successful: Boolean) {
		finalizers.asReversed().forEach { it(successful) }
	}
}
suspend fun TestDsl.cleanUp(
	name: String,
	onSuccess: Boolean = true,
	onFailure: Boolean = true,
	block: suspend TestDsl.() -> Unit,
) {
	environment.finalizers.register { successful ->
      if (!(successful && it.onSuccess || !successful && it.onFailure)) return@register
      println("» Finalizing '$name'")
      block()
    }
}

and you can easily see that Finalizers can be turned to an interface, and a ResourceScope-based implementation becomes very simple to write

[CLOVIS]

I still see them as a big upgrade for library uses since they let you efficiently have "extra null-like values" without degrading your code to using Any? and unsafe casts, or wrapping your values in `Option`s everywhere

100%. For that use-case, they seem amazing. However there's a lot more to error handling than just that.

[Youssef Shoaib [MOD]]

Coming back to this, I have to (somewhat) eat my words here! Raise, or at least a core implementation piece of it, is needed for Lexical Aborts in lifetimes! Of course, these are just design notes, but this means that a Raise-like feature is being discussed in an official KEEP! In fact, it's a key part of it, since the Stacks library cannot work without it. This also gives us the potential for better optimized Raise-ing code since lifetimes can allow for preventing allocations in some cases (and thus maybe some raise calls can turn into simple returns).

To clarify, if lifetimes were in Kotlin right now, a raise function like merge can be written very simply as follows:

fun interface Raise<in E> { fun raise(r: E): Nothing }

inline fun <R> merge(block: local Raise<R>.() -> R): R = block { return it }

(I'm using merge because it's the simplest raise builder, but all the other ones are expressible in terms of merge and some try-`catch`es).