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

cancel source task directly after timeout #462

Merged
merged 3 commits into from Nov 13, 2017

Conversation

Projects
None yet
2 participants
@Danny02
Contributor

Danny02 commented Nov 11, 2017

Why:
The source task may still suceed after a timeout if the backup task
does not complete immediatily,
e.g. et=execution time, source-et < timeout + backup-et

This is contrary to the documentation and the intended behaviour.

Side-effects:
This is a breaking change, because as stated above code will return
in some cases the result of a different task.

cancel source task directly after timeout
Why:
The source task may still suceed after a timeout if the backup task
does not complete immediatily,
e.g. et=execution time, source-et < timeout + backup-et

This is contrary to the documentation and the intended behaviour.

Side-effects:
This is a breaking change, because as stated above code will return
in some cases the result of a different task.

@Danny02 Danny02 force-pushed the Danny02:fix-timeout-source-cancelation branch from 1a3fbea to 533f5b9 Nov 11, 2017

@codecov

This comment has been minimized.

codecov bot commented Nov 11, 2017

Codecov Report

Merging #462 into master will increase coverage by 0.32%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #462      +/-   ##
==========================================
+ Coverage   88.85%   89.18%   +0.32%     
==========================================
  Files         351      351              
  Lines        9567     9525      -42     
  Branches     1254     1269      +15     
==========================================
- Hits         8501     8495       -6     
+ Misses       1066     1030      -36
@alexandru

This comment has been minimized.

Member

alexandru commented Nov 12, 2017

Interesting. I'm not convinced, your changes seem to be equivalent to the old code, but I have to look at those test cases more carefully.

If you're right, I'll merge it of course.

@Danny02

This comment has been minimized.

Contributor

Danny02 commented Nov 12, 2017

For example:

  • the source task takes 3 seconds to complete
  • the backup task takes 2 seconds
  • and the timeout is also 2 seconds

With the current implementation:

  • source will start at 0:00
  • backup will start (delayed by 2 seconds) at 0:02
  • source will finish at 0:03 while backup is still running

The result will be that the backup task will get canceled and the calculation continues with the result of the source task while the whole computation took 3 seconds which is longer as the specified timeout.

With my code change the source task would race against an already completed task (unit) which gets delayed by the specified timeout (don't know if delayedresult would be more performand).

So if the timeout is reached without the source task completing in time, it will get canceled before the backup task is even started. It also doesn't matter anymore how long the execution of the backup task takes.

@alexandru

This comment has been minimized.

Member

alexandru commented Nov 13, 2017

Ah, I understand now. You're right.

@alexandru alexandru merged commit 43546a3 into monix:master Nov 13, 2017

1 check passed

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

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

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