Skip to content
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

Add AutoClose module #3356

Merged
merged 12 commits into from
Jan 30, 2024
Merged

Add AutoClose module #3356

merged 12 commits into from
Jan 30, 2024

Conversation

nomisRev
Copy link
Member

Adds a new module with same support as Resource, but without requiring KotlinX or suspension.

I plan on splitting Resource from Arrow Fx, but we either do it here or we do it in 2.0 branch since there are some breaking changes in the Resource API. WDYT? I'm open for all ideas 😅

Some goals are to be able to use this in other libraries, and projects without having to introduce Arrow as a base dependency.

For example SuspendApp could include just AutoCloseable and Resource support but it doesn't need anything else by doing this we can provide a much more elegant SuspendApp API, whilst keeping everything working together nicely, etc. I've had the use for this pattern quite a lot since I started adopting this DSL approach for resources.

@nomisRev
Copy link
Member Author

According to our plan, this is the last non-bugfix release of the 1.x series.

From the release post, if this change is too disruptive we can also just put everything directly into 2.x.

cc\ @serras

Copy link
Contributor

github-actions bot commented Jan 23, 2024

@kyay10
Copy link
Collaborator

kyay10 commented Jan 23, 2024

@nomisRev Do we need to maintain source compatibility with the pre-existing impl? Or are we free to break some signatures?

@serras
Copy link
Member

serras commented Jan 23, 2024

I’m a bit worried about having both AutoCloseScope and Resource, because it creates the question of what to use in each moment.

This is very similar to what I faced in arrow-collectors, where both suspended and non-suspended collectors were needed. For me, the best option here would be to somehow unify both concepts, although maybe we can only do that at the level of API.

About splitting Resource, I’m not really sure that’s a good idea. The Coroutines library is already small, since we’ve split Resilience from it. My fear is also we end up with too many libraries (as I’ve seen happening in the Haskell world) and then people find it annoying to add too many of them.

settings.gradle.kts Outdated Show resolved Hide resolved
@serras
Copy link
Member

serras commented Jan 23, 2024

I've thought about it a bit more (bus rides give a lot of time for thinking), and I think we can easily explain why you need both concepts. I've noticed that ResourceScope extends AutoCloseScope, which I had missed in the previous review, and that actually sounds great.

I would go for including this in 1.2.2, and update the documentation about Resource management to talk about both at the same time.

@nomisRev
Copy link
Member Author

I’m a bit worried about having both AutoCloseScope and Resource, because it creates the question of what to use in each moment.

This IMO is quite simple, but I guess there is an essential piece of documentation missing.

  • AutoCloseable, originally a Java concept, has been added to Kotlin in 1.9.x. AutoCloseScope is a DSL for registering non-suspend finalisers, or composeable finally. Which is capable of allowing safely unwrapping AutoCloseable.
  • ResourceScope does practically the same but for suspend finalisers and cancellation.

Similar to how non-suspend function can be called from suspend function, can AutoCloseScope be called from ResourceScope but not the other way around.

Not sure if that clears it up, but I find this a reasonable and necessary split. In pure land, we would always use Resource but that heavily conflicts with what Java does, and thus most of the eco-system. However, I still find Resource necessary because in a lot of cases we need to deal with CancellationException in Kotlin when working with KotlinX Coroutines, etc.

About splitting Resource, I’m not really sure that’s a good idea. The Coroutines library is already small, since we’ve split Resilience from it. My fear is also we end up with too many libraries (as I’ve seen happening in the Haskell world) and then people find it annoying to add too many of them.

It's not completely explained in my original PR comment, but my idea is not to split but rather separate Resource into a separate module but still export it through arrow-fx-coroutines. I think that covers your concern, or not?

I have a personal use-case just for Resource in kotlin-kafka, and I really do not want to rely on Arrow & Arrow Fx for a library like that.

@nomisRev
Copy link
Member Author

nomisRev commented Jan 23, 2024

@serras saw your message too late 😅 We definitely need to include it into the docs, I will raise a PR tomorrow or the day after 👍

@nomisRev Do we need to maintain source compatibility with the pre-existing impl? Or are we free to break some signatures?

I was planning to take a look at 2.0 first, to see what the options are. So if you have any ideas, please just jump to 2.0 and feel free to make some breaking changes there if it results in a nicer API 😉

@serras
Copy link
Member

serras commented Jan 23, 2024

Do we need to maintain source compatibility with the pre-existing impl? Or are we free to break some signatures?

I would prefer that, as much as possible, Resource and AutoClose have the same API, except for suspend in some signatures. That would incredibly simplify the explanation of both concepts.

Copy link
Collaborator

@kyay10 kyay10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not very familiar with the resource dsl so I couldn't help much there.

@nomisRev nomisRev merged commit ae5ed95 into main Jan 30, 2024
11 checks passed
@nomisRev nomisRev deleted the sv-arrow-autocloseable branch January 30, 2024 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants