arrow icon indicating copy to clipboard operation
arrow copied to clipboard

Clean-up Resource for Arrow 2.0

Open nomisRev opened this issue 3 years ago β€’ 6 comments

This PR cleans up old code in Resource, and fully utilises the Dsl implementation as the underlying impl for Resource. Eliminating the need for the other ADT cases, simplifying the runtime and removing the need for a use runloop.

Removed APIs

  • zip: composing resource using resource { f(bind(), bind()) } supports arity-n
  • map/tap/flatMap: the signatures are about the same length, making them feel redundant for alias.
resA.flatMap {
  resB
}

resource {
  resA.bind()
  resB.bind()
}

resA.map(f)
resource { f(resA.bind()) }

resA.tap(f)
resource { resA.bind().also(f) }

New APIs / API changes

The Resource API now also includes a DslContext duality. Meaning we have top-level APIs that return a Resource, and we have DSL APIs which do the same but apply them inside the context of the DSL. The DSL APIs are highlighted as keywords using DslMarker.

In the example below you can see how we combine parZip with the resource DSL. And how we use release context DSL to apply the Resource inside the DSL. So we remove the need for calling bind.

Screenshot 2022-08-03 at 09 38 24

If you'd extract the Resource creation from within { } you could simply assign it to a val without changing the code, and it'll be reified as a Resource.

Screenshot 2022-08-03 at 09 40 18

This unifies the API from within the DSL, and outside the DSL. Whilst visually giving some IDEA aid to which is being used when.

nomisRev avatar Aug 03 '22 06:08 nomisRev

Design doc:

The internal implementation of Resource has heavily changed, and the public API has been adjusted to be in line with the DSL oriented changes we're making for Arrow 2.0.

Original impl

The Arrow 1.x Resource was originally implemented based on an ADT, and a tailrec interpreter but this is complex to implement. Hard to understand, and often doesn't play nice with types and IDEA etc. Secondly, this forced us to use flatMap etc like APIs.

Impl 2

The second implementation was inspired by Effect, and featured a DSL for writing Resource based programs. This was/is an in between solution, since this replaced the Defer and Bind cases of the ADT which could now be implemented in terms of the DSL.

This leaves us with only 2 ADT cases. Allocate and DSL. While DSL sequences the functions and keeps track of finalisers, and Allocate binds the resources and stores the finalisers inside the DSL.

Impl 3 (Arrow 2.0)

The DSL now implements Allocate in its algebra, and this completely removes the need for Allocate case.

This however opened the door to easily create incorrect sequenced resources, if we remember that acquire is NonCancellable you can easily make the entirety of the DSL NonCancellable by doing:

val resourceA: Resource<A> = resource { /* HUGE resource program */ }
resource {
   resource({
     resourceA.bind() // We make the entirety of resourceA uncancellable. DOES NOT COMPILE!
   }) { _, exitCase -> }.bind() 
}

This however is solved by leveraging @DSLMarker. While the previous "mistake" is still possible, the user would have to force himself to make this mistake. See below.

resource { outerScope ->
   resource({
      with(outerScope) {
        resourceA.bind() // We make the entirety of resourceA uncancellable. This example does compile.
     }
   }) { _, exitCase -> }.bind() 
}

So we consider this issue fixed, since it's not easily possible for a user to create an incorrect resource. Now that it only exists out of a single case, we can use typealias Resource<A> = suspend ResourceScope.() -> A as the definition.

Naming

The naming is just an arbitrary proposal!! Other things on the table have been Managed<A> & Manage. Resource is also still on the table, but we'd should find a better name than ResourceScope for the DSL. Any other suggestions very welcome πŸ™

Since the API has changed so much, and the internals have also changed quite a bit. I thought it would be useful to evaluate the naming again. I've updated the docs to use Scope and Scoped. Where Scoped is the lazy version of a scope... πŸ˜…

So scoped returns suspend Scope.() -> A, and scope executes a Scoped<A> so it returns A.

The lazy version is something that typically would not be used anymore. It's a pattern common in other language, for example Scala to assign functions to val but in Kotlin that is in general not that useful. So the following pattern is advised in the docs.

suspend fun Scope.executor(): ExecutorCoroutineDispatcher =
  fixedThreadPoolContext(16, "MyDatabasePool")

suspend fun Scope.dataSource(): DataSource =
  install({ DataSource(...) }) { ds, _ -> withContext(Dispatchers.IO) { ds.close() } }
  
fun main(): Unit = runBlocking {
  scope {
    val ctx = executor()
    val ds = dataSource()
    // program
  }
}

This result in much nicer code, and IMO much more readable. From the point of the caller you can just use this as regular code, they only need to be aware that it needs to be wrapped in scope.

nomisRev avatar Aug 09 '22 12:08 nomisRev

Just my opinion: I think Resource<A> is a good name, especially because resourceA.use { ... } maps very clearly to "I'm using a resource". But maybe ManagedScope works a bit better than ResourceScope...

serras avatar Aug 09 '22 13:08 serras

Thanks for sharing your thoughts @serras πŸ™Œ

I think Resource<A> is a good name, especially because resourceA.use { ... } maps very clearly to "I'm using a resource".

I agree, and Kotlin Std also has use for A : Closeable and A : AutoCloseable, but I removed use from the codebase because I wanted to start from scratch.. suspend Scope.() -> A is not really a Resource anymore, but represents a suspend computation that can install n resources and produces a value of A. Not sure if that is really important though for the naming.

The idea was that you would never need use anymore, but would encode your program using regular functions. Using either Scope as a receiver, or as a context(Scope), this DSL makes use a bit obsolete.

suspend fun <A, B> Scoped<A>.use(f: suspend (A) -> B): B = scope { f(bind()) }

Not saying we have to go with these drastic changes, but I do like the idea of just being able to write top-level functions for writing resource safe programs. And ofc we can already do the same today:

suspend fun ResourceScope.executor(): ExecutorCoroutineDispatcher = 
  Resource.fromCloseable { ... }.bind()

But maybe ManagedScope works a bit better than ResourceScope...

I like ManagedScope over ResourceScope but I think we should drop the Scope postfix like we did for EffectScope -> Shift. Renaming it after the abstract install also doesn't yield a nice name, "Installable"... I also thought of Impervious as in "leak proof", but found it too difficult word in English so I didn't even list it below.

typealias Resource<A> = suspend ManagedScope.() -> A
typealias Resource<A> = suspend Managed.() -> A
typealias Resource<A> = suspend Installable.() -> A

I really like this new API, even without use but naming it is super hard IMO.

nomisRev avatar Aug 09 '22 14:08 nomisRev

@serras, I can also revert the naming changes and then we can keep this PR contained to the API. All new signatures can be back-ported to 1.1.x, and then we can still evaluate the naming changes later on.

That would at least help speed up this PR, and would allow us to get a better feel for some of the APIs. WDYT?

nomisRev avatar Aug 09 '22 14:08 nomisRev

I really like your plan of back porting things to Arrow 1.x, to give people a feeling of the new API. Ultimately people have to learn some naming, so keeping the current one is also a good choice πŸ‘

serras avatar Aug 09 '22 16:08 serras

Yes, that makes more sense. I’m going to revert the naming changes πŸ‘

nomisRev avatar Aug 09 '22 16:08 nomisRev

@serras, not to block this PR but I have given this some more thought on the naming and I am back to leaning for Scope πŸ˜…

My rationale being the following: suspend Scope.() -> A offers similar DSL to suspend CoroutineScope.() -> A but where CoroutineScope only scopes Coroutine, Scope is a high-level abstraction that can model any Scope.

For example the CoroutineScope DSL can be derived from Scope:

context(Scope)
suspend fun coroutineScope(context: CoroutineContext): CoroutineScope =
  install({ CoroutineScope(context + Job()) }) { scope, ex ->
    when (ex) {
      ExitCase.Completed -> Unit
      is ExitCase.Cancelled -> scope.coroutineContext.job.cancel(cause = exitCase.exception)
      is ExitCase.Failure -> scope.coroutineContext.job.cancel(CancellationException(ex.failure.message, ex.failure))
    }
  }.bind()

suspend fun <A> coroutineScope(block: suspend CoroutineScope.() -> A): A = scope {
  val coroutineScope = coroutineScope(currentCoroutineContext())
  return block(coroutineScope).also {
    // Await all children jobs to complete
    coroutineScope.coroutineContext.job.children.forEach { it.join() }
  }
}

WDYT? Scope seems like a good name to me since Resource seems only a subset of the functionality this offers. Although you might also argue that all use-cases it covers are related to "resource management" πŸ€”but the scope name is something we already know from other parts in the Kotlin eco-system... I'm really jumping back-and-forth πŸ˜…

nomisRev avatar Aug 16 '22 12:08 nomisRev

In my mind it makes sense forScope to relate to resources, since any structured management of concurrency/threads/resources is somehow tied to managing the scopes where things are visible.

If I understand correctly, though, the naming was reversed to keep compatibility with the 1.x branch. To me backporting this PR feels less important than having a nice API in Arrow 2.x, but we can also put some changes in main and have this PR target arrow-2 and change names.

serras avatar Aug 16 '22 12:08 serras

I am also in favour of Scope given the points mentioned above

i-walker avatar Aug 16 '22 12:08 i-walker

To me backporting this PR feels less important than having a nice API in Arrow 2.x

Agreed, but most of these APIs can be very easily back ported to 1.x.x with minimal changes, and without breaking binary compatibility. That's why I suggested it. The sooner we can promote better APIs the better I think, unless it requires a lot effort or maintenance.

I just wanted to share my internal "struggle" with the naming πŸ˜…

If I understand correctly, though, the naming was reversed to keep compatibility with the 1.x branch This was done to unblock this PR. A potential name change can be unrelated to the implementation change.

But I think the review from @raulraja, and @i-walker were outdated. I requested re-review, and when approved this can be merged.

nomisRev avatar Aug 16 '22 13:08 nomisRev