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

Fix local isolate #973

Merged
merged 14 commits into from Aug 9, 2019

Conversation

@Avasil
Copy link
Collaborator

commented Aug 6, 2019

Fixes #966

Not finished but might be easier to discuss

Avasil added 3 commits Aug 6, 2019
Merge branch 'master' into fix-local-isolate
# Conflicts:
#	monix-eval/shared/src/main/scala/monix/eval/Task.scala

@Avasil Avasil added this to the 3.0.0 milestone Aug 6, 2019

@Avasil

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 8, 2019

@oleg-py Thank you for the review, I updated PR:

  • Removed duplicated test
  • used FutureUtils.transform
  • changed bindCurrentif to private[monix]

@Avasil Avasil changed the title WIP: Fix local isolate Fix local isolate Aug 8, 2019

Avasil added 2 commits Aug 8, 2019

@Avasil Avasil merged commit 7cd26d0 into monix:master Aug 9, 2019

1 check passed

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

left a comment

Sorry for the late review 🙂

/** Execute an asynchronous block of code without propagating any `Local.Context`
* changes outside.
*/
def isolate[R](f: => Future[R]): Future[R] = {

This comment has been minimized.

Copy link
@alexandru

alexandru Aug 9, 2019

Member

I am concerned about this overload.

The old one is a macro, this one is not. Also I know that the JVM cannot differentiate between R and Future[R], especially here because we have a by-name parameter which translates into a Function0.

I'm thinking that we should no longer use a macro here. I'm not convinced that we gain anything by it. Macros are fine for internal functions, were we don't want the overhead of that closure, but for user code it just makes things complicated.

So I'm thinking that maybe we should introduce a type class, because why not:

trait CanIsolate[R] {
  def isolate(f: => R): R
}

//...

object Local {
  def isolate[R : CanIsolate](f: => R): R =
    CanIsolate[R].isolate(f)
}

Given that what we had was a macro, I don't think this would violate any binary compatibility.

Not sure about how this should behave in the context of Task though. Or if it's a good idea. Just a suggestion.

This comment has been minimized.

Copy link
@Avasil

Avasil Aug 9, 2019

Author Collaborator

I agree with macro, I meant to rewrite it to macro later but only because of consistency. I think we could safely remove it (and possibly reintroduce if it turns out it's needed). I know Oleg kept it for similar reasons

So I'm thinking that maybe we should introduce a type class, because why not:
...

Hmm, I think it could work, we could have an instance for Future and then a lower priority instance for everything else. I can't tell if it's an improvement or not, to me it seems similar to having overload

Not sure about how this should behave in the context of Task though. Or if it's a good idea. Just a suggestion.

Task has a different function for isolate on TaskLocal which does bracket. We could add an instance for normal Local in Task object I guess if it will be higher priority than default one for Local (not sure, needs to check)

WDYT @oleg-py ?

This comment has been minimized.

Copy link
@oleg-py

oleg-py Aug 9, 2019

Collaborator

I have no preference here. Typeclasses will work as well as asInstanceOf, really, unless you push it to properly isolate Option[Future[Option[Future[A]]]]. You'll also have to make sure it works properly with our CancelableFuture, that it doesn't fall back to lower priority instance.

maybe we should introduce a type class, because why not

🤣

try f
finally Local.setContext(parent)

f match {

This comment has been minimized.

Copy link
@alexandru

alexandru Aug 9, 2019

Member

Note that this does not have the semantic of the original code — f is a by-name parameter and you're evaluating it twice.

Also the first time you're evaluating f, it isn't in the context of a try/catch statement. This means that if f throws an error, you're not restoring the context.

Also this pattern matching here is not future proof. It isn't idiomatic Scala so to speak. If we really need to discriminate between data types like this, then we should use implicits.

This comment has been minimized.

Copy link
@Avasil

Avasil Aug 9, 2019

Author Collaborator

Note that this does not have the semantic of the original code — f is a by-name parameter and you're evaluating it twice.
Also the first time you're evaluating f, it isn't in the context of a try/catch statement. This means that if f throws an error, you're not restoring the context.

Right, I will fix it

Also this pattern matching here is not future proof. It isn't idiomatic Scala so to speak. If we really need to discriminate between data types like this, then we should use implicits.

What do you mean by "future proof" ?

@Avasil Avasil deleted the Avasil:fix-local-isolate branch Aug 21, 2019

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