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

Fix Task.bracket, onCancelRaiseError; introduce Task.ContextSwitch #679

Merged
merged 14 commits into from May 22, 2018

Conversation

Projects
None yet
2 participants
@alexandru
Member

alexandru commented May 21, 2018

Changes operations:

  • bracket is fixed to work with auto-cancelable bind loops
  • onCancelRaiseError is fixed to reset the cancelation flag (as per the described laws in upcoming cats-effect 1.0.0-RC2)
  • uncancelable and autoCancelable are now more efficient due to the introduction of a new internal state, the Task.ContextSwitch, which is also used to make bracket not be totally terrible after the changes
  • add cancelBoundary for consistency with cats.effect.IO

Benchmarks:

Name 3.0.0-RC1 This PR
TaskCancelableBenchmark.bracketAsync 904.631 797.622
TaskCancelableBenchmark.bracketSync 1399.900 1198.619
TaskCancelableBenchmark.onCancelRaiseError 2157.638 2142.344
TaskCancelableBenchmark.uncancelable 2968.847 4930.648

Note that bracket takes a performance hit, however the version in this PR:

  1. works for auto-cancelable binds
  2. declares acquire and release to be uncancelable

So it does more, the performance hit being justifiable and not totally horrible.

alexandru added some commits May 16, 2018

Fix
@codecov

This comment has been minimized.

codecov bot commented May 22, 2018

Codecov Report

Merging #679 into master will increase coverage by <.01%.
The diff coverage is 95.53%.

@@            Coverage Diff             @@
##           master     #679      +/-   ##
==========================================
+ Coverage   91.14%   91.14%   +<.01%     
==========================================
  Files         384      384              
  Lines       10355    10417      +62     
  Branches     1931     1945      +14     
==========================================
+ Hits         9438     9495      +57     
- Misses        917      922       +5
@oleg-py

This comment has been minimized.

Collaborator

oleg-py commented May 22, 2018

Does having ContextSwitch state means we can improve Task.shift to continue execution on provided EC even after an async boundary?

@alexandru

This comment has been minimized.

Member

alexandru commented May 22, 2018

Yes 🙂 but I don’t want to do that 😛

alexandru added some commits May 22, 2018

@oleg-py

This comment has been minimized.

Collaborator

oleg-py commented May 22, 2018

Why? 😞

@alexandru alexandru merged commit 062af1b into monix:master May 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alexandru

This comment has been minimized.

Member

alexandru commented May 22, 2018

@oleg-py The ContextSwitch node I introduced allows for such behavior as an optimization, however it invalidates Task's design.

Task is designed to behave like a stack in its operators. It replaces Java's call-stack after all. For example executeOn only affects the source, but not the bind continuation (which is why I agreed to make it auto-shift back to the default scheduler). For example a shift that behaves like you want would conflict with this code:

val task = for {
  _  <- op1
  _ <- Task.shift(ec2)
  _ <- op2
} yield ()

task.executeOn(io)

Task.shift overriding the default scheduler for the bind continuation means that op2 in this piece of code will not receive io as the default scheduler, as indicated by executeOn. That is not intuitive because you effectively have a task in the bind chain that affects the behavior of the task following it, without that behavior being born out of a data dependency, as there is no data dependency here.

And you might think such a sample is cool, until you deal with functions that you haven't written yourself:

// Not your code
def foo: Task[Unit] =
  for {
    _  <- op1
    _ <- Task.shift(ec2)
    _ <- op2
  } yield ()

// Your code
for {
  _ <- foo
  _ <- bar
} yield ()

Is it obvious that bar will receive ec2 as the default scheduler? Of course not, it's a huge surprise and not something that can happen with usage of alternatives like executeOn.

Can you then correct its behavior with executeOn? Nope, you no longer can. The only thing you can do is to keep doing shift, but you've lost the real default and you can't get back to it.

@oleg-py

This comment has been minimized.

Collaborator

oleg-py commented May 22, 2018

@alexandru thanks for the explanation; fully agreed, and with current changes executeOn is already super nice.

But I might have a crazy suggestions to abuse ContextSwitch later on 😄

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