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 Fiber interface, refactor `memoize` #596

Merged
merged 19 commits into from Feb 22, 2018

Conversation

Projects
None yet
2 participants
@alexandru
Member

alexandru commented Feb 21, 2018

Changes:

  • adds Fiber interface, to be consistent with cats-effect
  • refactors Coeval and Task to no longer treat memoize as a special state in their ADT definition

Fiber gets used whenever a Task is attached to a result that will be available on the completion of an already running process. Resembles CancellableFuture in semantics, but gets used within the Task context, currently for:

  1. start
  2. pairBoth

This PR also refactors the memoize operation on both Coeval and Task. We are simplifying the ADTs, memoize no longer being represented as a special state. This simplifies the implementation of the run-loop, reducing the area for potential bugs. Thus, internally, Coeval.Once and Task.MemoizeSuspend are gone.

alexandru added some commits Jul 17, 2017

@alexandru alexandru added this to the 3.0.0 milestone Feb 21, 2018

@codecov

This comment has been minimized.

codecov bot commented Feb 21, 2018

Codecov Report

Merging #596 into master will increase coverage by <.01%.
The diff coverage is 93.9%.

@@            Coverage Diff             @@
##           master     #596      +/-   ##
==========================================
+ Coverage   90.74%   90.75%   +<.01%     
==========================================
  Files         373      375       +2     
  Lines        9902     9886      -16     
  Branches     1874     1878       +4     
==========================================
- Hits         8986     8972      -14     
+ Misses        916      914       -2
* _ <- runToBunker.handleErrorWith { error =>
* // Retreat failed, cancel launch (maybe we should
* // have retreated to our bunker before the launch?)
* fiber.cancel.flatMap(_ => IO.raiseError(error))

This comment has been minimized.

@oleg-py

oleg-py Feb 21, 2018

Collaborator

Should be Task instead of IO

@@ -736,6 +785,7 @@ object Coeval extends CoevalInstancesLevel0 {
def apply[A](f: => A): Coeval[A] =
Always(f _)
Stream(1,2,3).map(_ + 1)

This comment has been minimized.

@oleg-py

oleg-py Feb 21, 2018

Collaborator

Leftover?

@@ -26,12 +26,12 @@ import scala.util.Success
object TaskStartSuite extends BaseTestSuite {
test("task.start.flatMap(id) <-> task") { implicit sc =>

This comment has been minimized.

@oleg-py

oleg-py Feb 21, 2018

Collaborator

I think test descriptions need to be updated too 😏

val state = Atomic(null : AnyRef)
def isCachingAll: Boolean =
cacheErrors

This comment has been minimized.

@oleg-py

oleg-py Feb 21, 2018

Collaborator

Isn't this getter redundant, as cacheErrors is already a public field?

* value or to cancel the background process that calculates
* the memoized result (as long as it can be cancelled)
*/
final def memoize: Fiber[A] =

This comment has been minimized.

@oleg-py

oleg-py Feb 21, 2018

Collaborator

Excuse me if I'm totally wrong here

I don't really see how Fiber is a way to represent memoization. My intuition tells me that when I get a Fiber[A], the evaluation process is already running, so my options are either waiting for its completion or telling it that it's no longer necessary (which might still continue running, but I'll never get a result). But implementation of TaskMemoize seems to imply the evaluation will only happen when I'll try to perform a join, so it doesn't really represent a running background process.

This comment has been minimized.

@alexandru

alexandru Feb 22, 2018

Member

You're totally right, haven't thought this through. I'm reverting the change.

alexandru added some commits Feb 22, 2018

@alexandru

This comment has been minimized.

Member

alexandru commented Feb 22, 2018

Thanks for the review @oleg-py. Not modifying the .memoize signature.

@alexandru alexandru merged commit 2262297 into monix:master Feb 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 Feb 24, 2018

@Avasil I forgot to mention ...

Because of this PR I ended up reverting #555 unfortunately.

When we fork a Task now we get a Fiber back, which basically allows for cancellation.
This kind of makes it incompatible with CoflatMap, at least the signature.

We might get back to forking as the behavior for CoflatMap, but we'll have to wait to see what happens with the type class hierarchy in cats-effect, because we might end up with some MonadFork type class.

I mean, if nobody ends up using CoflatMap[Task] for forking (e.g. FS2, etc), then we shouldn't do it because task.fork.map(_.join) now doesn't sound so good.

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