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 Task.forkAndForget #530

Merged
merged 6 commits into from Jan 16, 2018

Conversation

Projects
None yet
3 participants
@Avasil
Collaborator

Avasil commented Jan 15, 2018

Fixes #520.
Implementation is very simple, it seems to work but I'm of course open to any suggestions improving it and I think ScalaDoc could use meaningful example which could potentially be also used in one of the tests.

@Avasil Avasil referenced this pull request Jan 15, 2018

Closed

Add Task.forkAndForget #520

@codecov

This comment has been minimized.

codecov bot commented Jan 15, 2018

Codecov Report

Merging #530 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #530      +/-   ##
=========================================
+ Coverage   90.38%   90.4%   +0.01%     
=========================================
  Files         362     364       +2     
  Lines        9610    9629      +19     
  Branches     1802    1797       -5     
=========================================
+ Hits         8686    8705      +19     
  Misses        924     924
// Starting actual execution of our newly created task forcing new async boundary
Task.unsafeStartAsync(fa, ctx2, Callback.empty)
// Signal the created Task reference
cb.onSuccess(())

This comment has been minimized.

@oleg-py

oleg-py Jan 15, 2018

Collaborator

Nitpick: the comment above is not correct, there's no Task reference signalled

@alexandru

There is now a fork method on Task that does executeAsync.start, see: #532

  • rename startAndForget to forkAndForget
  • fix ScalaDoc to mention fork instead of start
* }}}
*
* IMPORTANT - In contrast to [[start]] it does automatic fork,
* forcing new async boundary.

This comment has been minimized.

@alexandru

alexandru Jan 15, 2018

Member

Once you refer to fork instead of start, this comment is not needed.

// Starting actual execution of our newly created task forcing new async boundary
Task.unsafeStartAsync(fa, ctx2, Callback.empty)
// Signal the created Task reference
cb.onSuccess(())

This comment has been minimized.

@alexandru

alexandru Jan 15, 2018

Member

This has the potential of generating stack overflow errors under certain conditions, which is why as policy we never call cb.onSuccess(()) straight from Async.

Please use cb.asyncOnSuccess(()).

test("task.startAndForget is stack safe") { implicit sc =>
var task: Task[Any] = Task(1)
for (_ <- 0 until 5000) task = task.startAndForget
for (_ <- 0 until 5000) task = task.flatMap(_ => Task.unit)

This comment has been minimized.

@alexandru

alexandru Jan 15, 2018

Member

Use 5000 for JavaScript, 100000 for Java. You can differentiate by using Platform.isJVM (from monix.execution.internal).

test("task.startAndForget <-> task.start.map(_ => ())") { implicit sc =>
check1 { (task: Task[Int]) =>
task.startAndForget <-> task.start.map(_ => ())

This comment has been minimized.

@alexandru

alexandru Jan 15, 2018

Member

startAndForget on evaluation triggers side effects that we cannot notice here; this test is basically a no-op because you could've said:

task.startAndForget <-> Task.unit

We don't need it. Get rid of it.

This comment has been minimized.

@Avasil

Avasil Jan 15, 2018

Collaborator

Can I somehow prove this equivalence?

This comment has been minimized.

@alexandru

alexandru Jan 15, 2018

Member

I don't think so. Would be nice but forget is in the name 😀 and I can't think of anything useful to do here.

task.startAndForget.runAsync
sc.tick()
counter <-> i

This comment has been minimized.

@alexandru

alexandru Jan 15, 2018

Member

You're using ScalaCheck to repeat the test for random i: Int values, however that won't do much for us.

Do a simple test and test that:

  1. async boundary is needed
  2. execution is actually triggered in a background process (its own thread)
val task = Task.eval { counter += 1; counter }
val main = for {
  _ <- task.delayExecution(1.second).forkAndForget // first
  _ <- task.delayExecution(1.second).forkAndForget // second
} yield ()

val f = main.runAsync
assertEquals(f.value, Some(Success(()))
assertEquals(counter, 0)

// Testing that both got executed in parallel, otherwise
// the total execution time would take 2 seconds
s.tick(1.second)
assertEquals(counter, 2)

Well, this doesn't test that an async boundary is forced, since we are doing it anyway with delayExecution, but you get the idea.

@Avasil

This comment has been minimized.

Collaborator

Avasil commented Jan 15, 2018

@alexandru Thanks, I've used your example and added similar test but with raising exception. Maybe after sleep I will come up with a way to test for async boundary, it seems like other tests use either delayExecution or executing on different Scheduler

@Avasil Avasil changed the title from Add Task.startAndForget to Add Task.forkAndForget Jan 16, 2018

@alexandru

Looks good. I'll go ahead and merge it tonight — if by then you can also add the assertion on lastReportedError that I mentioned above, that's cool, otherwise I'll merge it without it to not drag it any longer.

The tests are sufficient IMO. It's a hard to test operation since you can only observe side effects.

Thanks @Avasil

val ctx2 = Task.Context(sc, ctx.options)
// Starting actual execution of our newly created task forcing new async boundary
Task.unsafeStartAsync(fa, ctx2, Callback.empty)
cb.asyncOnSuccess(())

This comment has been minimized.

@alexandru

alexandru Jan 16, 2018

Member

Implementation looks good 👍

val f = result.runAsync
sc.tick()
assertEquals(f.value, Some(Success(20)))

This comment has been minimized.

@alexandru

alexandru Jan 16, 2018

Member

You're raising the error in that background thread, however you now need to ensure that the error was reported. Something like:

assertEquals(sc.state.lastReportedError, dummy)

This comment has been minimized.

@alexandru

alexandru Jan 16, 2018

Member

This happens due to Callback.empty if you were wondering, which isn't so "empty" 😀 as it reports incoming errors with the provided Scheduler.

@alexandru alexandru merged commit 13def28 into monix:master Jan 16, 2018

1 check passed

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

@Avasil Avasil deleted the Avasil:Task#startAndForget branch Jan 21, 2018

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