-
Notifications
You must be signed in to change notification settings - Fork 443
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.
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
Clean-up Resource for Arrow 2.0 #2786
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏🏾 👏🏾 Thanks for these huge improvements
timeout: Duration = Duration.INFINITE, | ||
closingDispatcher: CoroutineDispatcher = Dispatchers.IO, | ||
create: suspend () -> ExecutorService, | ||
): Resource<CoroutineContext> = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
way more refined 👏🏾
acquire: suspend () -> A, | ||
release: suspend (A, ExitCase) -> Unit, | ||
): Resource<A> = Allocate(acquire, release) | ||
public fun <A> resource(block: suspend ResourceScope.() -> A): Resource<A> = block |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏾
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type alias Resource definition is 🔥 as well as functions like use
, resource
and other internals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a great improvement to Resource ergonomics 💘
Design doc: The internal implementation of Original implThe 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. Impl 2The second implementation was inspired by This leaves us with only 2 ADT cases. Impl 3 (Arrow 2.0)The DSL now implements This however opened the door to easily create incorrect sequenced resources, if we remember that 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 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. NamingThe naming is just an arbitrary proposal!! Other things on the table have been 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 So 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 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 |
Just my opinion: I think |
Thanks for sharing your thoughts @serras 🙌
I agree, and Kotlin Std also has The idea was that you would never need 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()
I like 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 |
@serras, I can also revert the naming changes and then we can keep this PR contained to the API. That would at least help speed up this PR, and would allow us to get a better feel for some of the APIs. WDYT? |
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 👍 |
Yes, that makes more sense. I’m going to revert the naming changes 👍 |
… sv-clean-up-resource
@serras, not to block this PR but I have given this some more thought on the naming and I am back to leaning for My rationale being the following: For example the 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? |
In my mind it makes sense for 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 |
I am also in favour of |
Agreed, but most of these APIs can be very easily back ported to I just wanted to share my internal "struggle" with the naming 😅
But I think the review from @raulraja, and @i-walker were outdated. I requested re-review, and when approved this can be merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌🏾 🎊 Thanks for this. It is a great improvement
* Flatten Resource ADT, maintain API
* The beginning of Arrow 2.0 * Resource Arrow 2.0 (#2786) * Flatten Resource ADT, maintain API * [Arrow 2.0] Effect without suspending shift (#2797) * Shift without suspend, inline all the rest * Add new error handlers signatures * Make ShiftCancellationException private * Rename Shift to Raise according to Slack Poll, and add some initial docs (#2827) * Remove all references to shift from new Arrow 2.0 code (#2834) * Remove all references to shift from new code * Update API files * Fixes merge conflict between main and arrow-2 (#2835) * Add Resource.allocated() to decompose Resource into it's allocate and… (#2820) * [2743] Migrate internal use of CircuitBreaker double to duration (#2748) * Fix typo (#2824) * Make the server disconnect test more general (#2822) * Update NonEmptyList.fromList deprecation to suggest `toOption()` instead (#2832) * Improve Either.getOrHandle() docs (#2833) * Improve allocated, and fix knit examples Co-authored-by: Jeff Martin <jeff@custommonkey.org> Co-authored-by: Martin Moore <martinmoorej@gmail.com> Co-authored-by: valery1707 <valery1707@gmail.com> Co-authored-by: Lukasz Kalnik <70261110+lukasz-kalnik-gcx@users.noreply.github.com> Co-authored-by: stylianosgakis <stylianos.gakis98@gmail.com> * Add Atomic module, and StateShift (#2817) * Two small deprecations * Add Atomic module, and StateShift. Implement ior through StateShift * Fix build * Fix atomic knit * Fix knit attempt #2 * Update API files * Remove references to shift * Change return type of raise to Nothing (#2839) * Change return type of raise to Nothing Implements #2805 * Update public api with ./gradlew apiDump * Suppress warnings for unreachable calls to fail in tests The test is verifying that no code will be executed after a call to raise, so it's actually testing the very behaviour that the compiler is warning us about. Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Update API files * Increase timeout * Fix compiler bug with nested inline + while + return * Clean up ExitCase.fromError * Update API files@ * Feature/remove validated (#2795) Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com> Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Remove CancellationExceptionNoTrace.kt after merge, and run apiDump * Add missing runnerJUnit5 for arrow-atomic JVM * Publish Arrow 2.0.0-SNAPSHOT (#2871) Co-authored-by: Javier Segovia Córdoba <7463564+JavierSegoviaCordoba@users.noreply.github.com> * Simplify optics to Traversal/Optional/Lens/Prism (#2873) * 'mapOrAccumulate' for Raise (#2872) Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com> * Fix problems with Atomic * Smaller timeouts * Remove Tuple10 to Tuple22, Const, Eval, computation blocks, and arrow-continuations (#2784) * Revert typo * Fix build * Fix ParMapJvmTest * Implement NonEmptyList using value class (#2911) * Fix merge w.r.t. Saga * apiDump * Test other return expression * change unalign signature (#2972) see discussion in #2960 (comment) #2960 (comment) --------- Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Update after merge main * Fix :arrow-optics-ksp-plugin:compileKotlin * Fix Every instances * Move functions to arrow functions (#3014) * Bring back `Iso` (#3013) * Bring back Iso * API Dump --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Update BOM (#3019) * Fix andThen duplication (#3020) * Fix Knit * Fix weird problem with value classes * Update API docs * Update publish workflow Following the instructions in #3090 (comment) * No closing repo on snapshot * knit * Fix optics tests * Fix after merge * Refactor ParMapTest from Kotest Plugin to Kotlin-test runtime #3191 (#3221) Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com> Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Refactor: Use Kotlin-test runtime for arrow-fx-stm tests (#3224) Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Update all Gradle files to mention kotlin.test * Refactor ParZip2Test from Kotest Plugin to Kotlin-test runtime #3192 (#3222) Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Refactor ParZip3Test from Kotest Plugin to Kotlin-test runtime #3193 (#3223) Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Refactor GuaranteeCaseTest to use kotlin test (#3226) Closes #3190 * refactor: migrate NotEmptySetTest to kotlin-test (#3230) Co-authored-by: Alejandro Serrano <trupill@gmail.com> * refactor: migrate EagerEffectSpec to kotlin-test (#3233) Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Refactor NullableSpec from Kotest Plugin to Kotlin-test runtime (#3236) #3153 Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Refactor BracketCaseTest to use kotlin test (#3237) Closes #3186 Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Move arrow-functions tests to kotlin.test (#3243) * Inline `AtomicBoolean` (#3240) * Inline AtomicBoolean * Update API files --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * refactor: migrate MappersSpec to kotlin-test (#3248) * Refactor ResourceTestJvm from Kotest Plugin to Kotlin-test runtime (#3244) Closes #3213 * refactor: migrate FlowJvmTest to Kotlin-test (#3228) * Refactor ParZip9JvmTest from Kotest Plugin to Kotlin-test runtime (#3245) Closes #3211 * Refactor ParZip8JvmTest from Kotest Plugin to Kotlin-test runtime (#3246) Closes #3210 * refactor: migrate NumberInstancesTest to kotlin-test (#3232) * refactor: OptionTest to kotlin-test runtime (#3229) * Revert "Inline `AtomicBoolean` (#3240)" (#3279) This reverts commit a6f1e73. * Refactor ParZip6JvmTest from Kotest Plugin to Kotlin-test runtime (#3255) Closes #3208 * Refactor ParZip5JvmTest from Kotest Plugin to Kotlin-test runtime (#3256) Closes #3207 * Refactor ParZip3JvmTest from Kotest Plugin to Kotlin-test runtime (#3258) Closes #3205 * Refactor ParZip2JvmTest from Kotest Plugin to Kotlin-test runtime (#3259) Closes #3204 * Refactor ParMapJvmTest from Kotest Plugin to Kotlin-test runtime (#3260) Closes #3203 * Refactor ParZip4JvmTest from Kotest Plugin to Kotlin-test runtime (#3257) Closes #3206 * refactor: migrate RaiseAccumulateSpec to kotlin-test (#3250) * Refactor ParZip7JvmTest from Kotest Plugin to Kotlin-test runtime (#3247) Closes #3209 Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Update ComparisonKtTest.kt (#3274) Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com> * Update OptionSpec.kt (#3271) Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com> * Update TraceJvmSpec.kt (#3276) * Update TraceJvmSpec.kt * Update TraceJvmSpec.kt --------- Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com> * Update ParZip9Test.kt (#3265) * Update ParZip9Test.kt * Update ParZip9Test.kt * Update ParZip8Test.kt (#3266) * Update ParZip7Test.kt (#3267) * Update ParZip6Test.kt (#3268) * Update ParZip5Test.kt (#3269) * Update ParZip4Test.kt (#3270) * Refactor CountDownLatchSpec and CyclicBarrierSpec to use kotlin test (#3227) * Refactor CountDownLatchSpec to use kotlin test Closes #3187 * Refactor CyclicBarrierSpec to use kotlin test Closes #3188 --------- Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Refactor NonEmptyListTest to kotlin-test (#3231) * Add kotlin test dependency * Refactor NonEmptyList Test to use kotlin test --------- Co-authored-by: Alejandro Serrano <trupill@gmail.com> * refactor: migrate EffectSpec to kotlin-test (#3234) * Refactor FlowTest to use kotlin test (#3238) Closes #3189 Co-authored-by: Alejandro Serrano <trupill@gmail.com> * refactor: migrate IorSpec to kotlin-test (#3249) Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com> * refactor: migrate ResultSpec to kotlin-test (#3251) Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com> * refactor: migrate StructuredConcurrencySpec to kotlin-test (#3252) * refactor: migrate TraceSpec to kotlin-test (#3253) * refactor: migrate GeneratorsTest to kotlin-test (#3254) * Refactor RaceNJvmTest from Kotest Plugin to Kotlin-test runtime (#3261) Closes #3212 Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Update ArrowResponseEAdapterTest.kt (#3264) * Update ArrowResponseEAdapterTest.kt * Update ArrowResponseEAdapterTest.kt --------- Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com> Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Update CollectionsSyntaxTests.kt (#3273) * Update CollectionsSyntaxTests.kt * Update CollectionsSyntaxTests.kt --------- Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com> Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Update NonFatalJvmTest.kt (#3277) * Update ArrowEitherCallAdapterTest.kt (#3278) * Update ArrowEitherCallAdapterTest.kt * Update ArrowEitherCallAdapterTest.kt * Move tests from `serialization` and `functions` completely to `kotlin.test` (#3289) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Fix problems with tests * Remove a bunch of warnings in `arrow-2` (#3282) Co-authored-by: serras <serras@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Finish transition to `kotlin.test` of `retrofit` and `fx-coroutines` (#3291) * Fix problems with concurrency in tests, take 8 * Port rest of `arrow-core` to `kotlin.test` (#3292) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Implement `fixedRate` using monotonic time source (#3294) Co-authored-by: serras <serras@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Port `optics` tests to `kotlin.test` (#3295) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Add or fix contracts in Raise (#3293) * Add or fix contracts in Raise * Make contracts less strict * Get back contract on Either * ignoreErrors should also be AT_MOST_ONCE --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Alternate `SingletonRaise` (#3328) Co-authored-by: Youssef Shoaib <canonballt@gmail.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: serras <serras@users.noreply.github.com> * Remove (unused) tests for high-arity module * Fix tests + Knit * Fix merge NullableSpec * Regression in Arb.list? * Fix test for nonEmptyList * Develocity warning * Fix merge problem with optics-ksp-plugin * Fix timeout in test --------- Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com> Co-authored-by: Sam Cooper <54266448+roomscape@users.noreply.github.com> Co-authored-by: Marc Moreno Ferrer <ignis.fatue@gmail.com> Co-authored-by: Javier Segovia Córdoba <7463564+JavierSegoviaCordoba@users.noreply.github.com> Co-authored-by: Alphonse Bendt <370821+abendt@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Andreas Storesund Madsen <andreas@asmadsen.no> Co-authored-by: molikuner <molikuner@gmail.com> Co-authored-by: Jonathan Lagneaux <lagneaux.j@gmail.com> Co-authored-by: Marcus Ilgner <mail@marcusilgner.com> Co-authored-by: Chris Black <2538545+chrsblck@users.noreply.github.com> Co-authored-by: Tejas Mate <hi.tejas@pm.me> Co-authored-by: HyunWoo Lee (Nunu Lee) <54518925+l2hyunwoo@users.noreply.github.com> Co-authored-by: serras <serras@users.noreply.github.com> Co-authored-by: Youssef Shoaib <canonballt@gmail.com>
This PR cleans up old code in
Resource
, and fully utilises theDsl
implementation as the underlying impl forResource
. Eliminating the need for the other ADT cases, simplifying the runtime and removing the need for ause
runloop.Removed APIs
resource { f(bind(), bind()) }
supports arity-nresA.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 aResource
, 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 usingDslMarker
.In the example below you can see how we combine
parZip
with theresource
DSL. And how we userelease
context DSL to apply theResource
inside the DSL. So we remove the need for calling bind.If you'd extract the
Resource
creation from within{ }
you could simply assign it to aval
without changing the code, and it'll be reified as aResource
.This unifies the API from within the DSL, and outside the DSL. Whilst visually giving some IDEA aid to which is being used when.