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

implement bracket for resource usage #137

Open
sledorze opened this Issue Aug 2, 2018 · 7 comments

Comments

Projects
None yet
2 participants
@sledorze

sledorze commented Aug 2, 2018

I put that quickly in shape.
I've done some (Unit) tests that are passing.. (including cancellation)

I'm wondering if I'm on the right path of if there's a fundamental flaw in it?

const bracket = <A>(acquire: IO<A>) => (release: (a: A) => IO<void>) => <B>(
  utilize: (a: A) => IO<B>
): IO<B> =>
  acquire.chain(resource => {
    const doRelease = release(resource)
    return utilize(resource)
      .doOnCancel(doRelease)
      .doOnFinish(_ => doRelease)
  })
@alexandru

This comment has been minimized.

Show comment
Hide comment
@alexandru

alexandru Aug 3, 2018

Member

We indeed need to update Funfix's IO with the latest developments, which includes bracket.

Your proposal might work for now, but note that:

  1. acquire has to be uncancelable
  2. release has to be uncancelable
  3. I don't remember if doOnCancel does the right thing, but we need to ensure that either doOnCancel or doOnFinish execute and not both

We would also need a bracketCase that discriminates between the exit cases, like we pushed in Cats-Effect.

I'm caught up with work on Monix and Cats-Effect at the moment, unfortunately. Plus work and life, I did not have any time left for Funfix.

Once Cats-Effect 1.0 and Monix 3.0 will be finally out, I hope to have some time for giving Funfix some needed updates.

Until then PRs are welcome in case you'd like to give it a try.

Member

alexandru commented Aug 3, 2018

We indeed need to update Funfix's IO with the latest developments, which includes bracket.

Your proposal might work for now, but note that:

  1. acquire has to be uncancelable
  2. release has to be uncancelable
  3. I don't remember if doOnCancel does the right thing, but we need to ensure that either doOnCancel or doOnFinish execute and not both

We would also need a bracketCase that discriminates between the exit cases, like we pushed in Cats-Effect.

I'm caught up with work on Monix and Cats-Effect at the moment, unfortunately. Plus work and life, I did not have any time left for Funfix.

Once Cats-Effect 1.0 and Monix 3.0 will be finally out, I hope to have some time for giving Funfix some needed updates.

Until then PRs are welcome in case you'd like to give it a try.

@sledorze

This comment has been minimized.

Show comment
Hide comment
@sledorze

sledorze Aug 3, 2018

About 1 and 2, is there a way to enforce this?
About 3, I've made the tests and it works.
I Maybe do a PR, but I'm not versed into Flow..

sledorze commented Aug 3, 2018

About 1 and 2, is there a way to enforce this?
About 3, I've made the tests and it works.
I Maybe do a PR, but I'm not versed into Flow..

@sledorze

This comment has been minimized.

Show comment
Hide comment
@sledorze

sledorze Aug 3, 2018

I've seen cancelable IO are IOAsync which have the cancellation logic in their Context.
On may use a constraint using the '_tag' value to enforce statically correct usage of the API but I find that sloppy.

SO I guess, I just need to make them become uncancellable in the implementation of bracket.

sledorze commented Aug 3, 2018

I've seen cancelable IO are IOAsync which have the cancellation logic in their Context.
On may use a constraint using the '_tag' value to enforce statically correct usage of the API but I find that sloppy.

SO I guess, I just need to make them become uncancellable in the implementation of bracket.

@alexandru

This comment has been minimized.

Show comment
Hide comment
@alexandru

alexandru Aug 3, 2018

Member

Don't we have an uncancelable operation already?

Member

alexandru commented Aug 3, 2018

Don't we have an uncancelable operation already?

@sledorze

This comment has been minimized.

Show comment
Hide comment
@sledorze

sledorze Aug 3, 2018

@alexandru Not found one actually..

sledorze commented Aug 3, 2018

@alexandru Not found one actually..

@sledorze

This comment has been minimized.

Show comment
Hide comment
@sledorze

sledorze Aug 3, 2018

Here's the issue: #138

sledorze commented Aug 3, 2018

Here's the issue: #138

@alexandru

This comment has been minimized.

Show comment
Hide comment
@alexandru

alexandru Aug 3, 2018

Member
Member

alexandru commented Aug 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment