Skip to content
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

Scheduler.features #960

Merged
merged 22 commits into from Aug 2, 2019

Conversation

@alexandru
Copy link
Member

commented Jul 27, 2019

The primary purpose of this PR is to make Observable play well with Local and TaskLocal when used in combination with Task (e.g. in Observable.mapEval).

For this purpose it adds a def features property to Scheduler.

This helps with discovery of what the underlying scheduler supports, because instanceOf checks are problematic to say the least due to all the wrapping. Features that can be exposed by schedulers currently:

  • BATCHING — special handling of TrampolinedRunnable, for schedulers that inherit from BatchingScheduler
  • TRACING — handling of Local, implemented by TracingScheduler and TracingSchedulerService

With a Scheduler instance you might be tempted to do this:

// WRONG
if (scheduler.isInstanceOf[TracingScheduler]) {
  //...
}

However this is wrong for one because TracingSchedulerService does not inherit from TracingScheduler and also because people might implement their own Scheduler wrappers and implementations that might use an underlying TracingScheduler, which would support Local, but which would fail an isInstanceOf[TracingScheduler].

With a Scheduler instance you can now do this:

if (scheduler.features.contains(Scheduler.TRACING)) {
  // ...
}

Using this, Task.runAsync can now activate the "local context propagation" in case the Scheduler is a TracingScheduler:

implicit val scheduler = TracingScheduler(global)

// This will have the "local context propagation" active
task.runAsync

This helps in Observable, because Observable's mapEval does not take the local context propagation into account:

Observable(1, 2, 3).mapEval(task).completedL.runAsyncOpt

Currently the tasks evaluated inside of mapEval won't take any "local context propagation" option enabled via runAsyncOpt, this being IMO an API bug.

alexandru added 9 commits Jan 21, 2019

@alexandru alexandru requested review from oleg-py and Avasil Jul 27, 2019

alexandru added 2 commits Jul 27, 2019
@alexandru

This comment has been minimized.

Copy link
Member Author

commented Jul 27, 2019

@mdedetrich would love your input as well.

alexandru added 4 commits Jul 28, 2019

@alexandru alexandru changed the title [WIP] Scheduler.features Scheduler.features Jul 28, 2019

@alexandru alexandru added this to the 3.0.0 milestone Jul 28, 2019

alexandru added 2 commits Jul 28, 2019
@@ -2597,7 +2597,7 @@ abstract class Observable[+A] extends Serializable { self =>
* throws an error.
*/
final def onErrorRecover[B >: A](pf: PartialFunction[Throwable, B]): Observable[B] =
onErrorHandleWith(ex => (pf.andThen(Observable.now(_))).applyOrElse(ex, Observable.raiseError _))
onErrorHandleWith(ex => (pf.andThen(Observable.now)).applyOrElse(ex, Observable.raiseError))

This comment has been minimized.

Copy link
@Avasil

Avasil Jul 28, 2019

Collaborator

AFAIK it won't compile with latest Scala 2.13 but it's fine to keep it unless you will have extra changes to do, I can fix it in PR updating to 2.13.0

@Avasil
Avasil approved these changes Jul 28, 2019
Copy link
Collaborator

left a comment

I'm personally fine with the change but I'm interested in @oleg-py opinion

@oleg-py
Copy link
Collaborator

left a comment

LGTM 👍

@mdedetrich

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

@alexandru Will look at it in the next couple of days, I fell sick last week so wasn't really available

alexandru added 3 commits Jul 31, 2019

@Avasil Avasil merged commit 28da9ce into monix:master Aug 2, 2019

1 check passed

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

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2019

I had a brief look at this PR last week, and its unrelated to the issues I am experiencing. We are not using Observable at all.

I definitely agreed with the direction of the PR though, for library authors its great to see what features the Scheduler has reliably (and I think I could use this feature in my monix-mdc/monix-opentracing to warn users if they don't have TracingScheduler enabled)

@alexandru

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2019

@mdedetrich this PR is also enabling TaskLocal to work in the presence of a TracingScheduler via runAsync. So if you're using a TracingScheduler, you no longer need to do a runAsyncOpt with the right Task.Options.

@alexandru alexandru deleted the alexandru:scheduler-features branch Aug 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.