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

Changed implementation TaskLocal#bind in terms of Task#bracket #603

Merged
merged 8 commits into from Mar 14, 2018

Conversation

Projects
None yet
3 participants
@leandrob13
Contributor

leandrob13 commented Feb 26, 2018

Addresses #601

Changes the TaskLocal#bind implementation in terms of Task#bracket which handles any cleanup of resources upon success, failure or cancellation.

def bind[R](value: A)(task: Task[R]): Task[R] =
    Task.suspend {
      val saved = ref.value
      ref.update(value)
      task.bracket(r => Task.now(r))(_ => restore(saved))
    }

leandrob13 added some commits Feb 26, 2018

@codecov

This comment has been minimized.

codecov bot commented Feb 26, 2018

Codecov Report

Merging #603 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #603      +/-   ##
==========================================
- Coverage   90.85%   90.81%   -0.04%     
==========================================
  Files         377      377              
  Lines        9979     9975       -4     
  Branches     1876     1882       +6     
==========================================
- Hits         9066     9059       -7     
- Misses        913      916       +3

alexandru added some commits Feb 27, 2018

@@ -134,7 +134,7 @@ final class TaskLocal[A] private (default: => A) {
Task.suspend {
val saved = ref.value
ref.update(value)
task.doOnFinish(_ => restore(saved))
task.bracket(r => Task.now(r))(_ => restore(saved))

This comment has been minimized.

@oleg-py

oleg-py Feb 27, 2018

Collaborator

If you want to handle cancellation, task has to be used inside bracket. E.g.

Task.unit.bracket(_ => task)(_ => restore(saved))

Also bindL needs to have similar change, so maybe it's easier to implement bind as bindL(Task.now(value))

This comment has been minimized.

@leandrob13

leandrob13 Feb 28, 2018

Contributor

@oleg-py understood!

@leandrob13

This comment has been minimized.

Contributor

leandrob13 commented Feb 28, 2018

@oleg-py So I redefined bindL like this:

def bindL[R](value: Task[A])(task: Task[R]): Task[R] = {
    val saved = ref.value
    value.bracket { v =>
      ref.update(v)
      task
    }(_ => restore(saved))
  }

Also, took care of bindClear too.

task.doOnFinish(_ => restore(saved))
}
def bindL[R](value: Task[A])(task: Task[R]): Task[R] = {
val saved = ref.value

This comment has been minimized.

@alexandru

alexandru Feb 28, 2018

Member

This ref.value should be suspended as well ...

Task.eval(ref.value).flatMap { saved =>
  value.bracket {
    ref.update(v)
    task
  } { _ =>
    restore(saved)
  }
}
ref.update(value)
task.doOnFinish(_ => restore(saved))
}
Task.suspend(bindL(Task.now(value))(task))

This comment has been minimized.

@alexandru

alexandru Feb 28, 2018

Member

If you suspend that value as I mentioned below, you don't need to suspend here.

This comment has been minimized.

@leandrob13

leandrob13 Feb 28, 2018

Contributor

@alexandru Understood, will take care of it later today.

leandrob13 added some commits Mar 1, 2018

@leandrob13

This comment has been minimized.

Contributor

leandrob13 commented Mar 2, 2018

@alexandru should I trigger another build?

@alexandru

This comment has been minimized.

Member

alexandru commented Mar 9, 2018

Please merge with master.

@alexandru

This comment has been minimized.

Member

alexandru commented Mar 14, 2018

Not 100% sure, but I think this is fine. Will merge for now.

@alexandru alexandru merged commit 708345e into monix:master Mar 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment