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

Use type classes instead of overloads in Local #977

Merged
merged 18 commits into from Aug 20, 2019

Conversation

@Avasil
Copy link
Collaborator

commented Aug 9, 2019

Proof of concept with:

  • Local macros removed
  • Added CanIsolate type class which describes bind and isolate

Comment from Alex's prs:

Local should have side-effectful behavior, should not suspend execution, otherwise its usage is misleading.

I've added an import for synchronous behavior:

import monix.execution.misc.CanBindLocals.Implicits.synchronousAsDefault

What this does is to import in scope an implicit for all unknown CanBindLocals[A] types, but that does NOT override the implicits that are already in scope. This is important because we don't want to override the Future instance via an import like this.

Towards this purpose I've published a new micro library that introduces the Not data type, for providing evidence that an implicit does not exist for a certain type:
https://github.com/monix/implicitbox

This is a very light dependency and introducing it in Monix felt wrong.

@Avasil Avasil changed the title Protect from error in Local.bind Use type classes instead of overloads in Local Aug 10, 2019

}

/** Execute a block of code using the specified state of
* `Local.Context` and restore the current state when complete.
*/
def bind[R](ctx: Context)(f: => R): R =
macro Macros.localLet
def bind[R: CanIsolate](ctx: Context)(f: => R): R = {

This comment has been minimized.

Copy link
@Avasil

Avasil Aug 10, 2019

Author Collaborator

I don't know why mima complains about this change but not for isolate

This comment has been minimized.

Copy link
@alexandru

alexandru Aug 11, 2019

Member

It complains because this is an actual method, whereas isolate is a macro.

Changing isolate right now will not break binary compat.

This means that this bind needs to be deprecated. Can you try the hard source deprecation trick via private[Local]?

This comment has been minimized.

Copy link
@Avasil

Avasil Aug 11, 2019

Author Collaborator

Not sure what's the trick but I think I managed to get rid of binary incompatibility without exposing old methods

@Avasil

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 10, 2019

@alexandru @oleg-py I made a PoC with type class CanIsolate. I think it's ok. Actually I noticed that there could be also a problem with bind which would require overload so type classes seem more elegant than next versions/overloads.

I didn't do it for Task for two reasons:

  1. Wanted to show something as soon as possible.
  2. I'm not sure whether we should encourage users to use Local methods with Task. TaskLocal has nicer, a bit higher level interface so maybe less advanced users should stick to using this one.
Avasil added 2 commits Aug 10, 2019

@Avasil Avasil requested a review from oleg-py Aug 11, 2019

project/MimaFilters.scala Outdated Show resolved Hide resolved
@alexandru

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

@Avasil

I'm not sure whether we should encourage users to use Local methods with Task. TaskLocal has nicer, a bit higher level interface so maybe less advanced users should stick to using this one.

Note that sometimes usage of Local in combination with Task is fine, as a low-level implementation detail. It's what Quill does 😉

Given that isolate or bind usage fails for Future, it means that they'll fail for any other type that can be waited on. So actually I'm thinking that we should disallow by default code like this, meaning the default implicit should not be available without an import:

// Should fail at compile time, unless we import something explicitly like:
import monix.execution.Local.Implicits.synchronous

Local.isolate {
  local.get
}

And maybe we can allow Unit by default, however we to take into account what happens in case somebody does something like this:

def printIt(): Unit =
  someContext.execute(() => {
    println(local.get)
  })

local := 100

context.execute(() => {
  Local.isolate {
    local := 200
    // Will this see 100 or 200?
    printIt()
  }

  // Will this see 100 or 200?
  printIt()
})
@alexandru

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

In other words, I'm thinking it might be a good idea to only have global implicits for Future, CompletableFuture, Coeval, Task and Observable, because for these we can wait for when the computation described by f: => R is done such that we can revert the context after isolate or bind.

Avasil and others added 2 commits Aug 11, 2019
Merge branch 'locals-protect-from-error' of git://github.com/Avasil/m…
…onix into Avasil-locals-protect-from-error
@Avasil

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 11, 2019

@alexandru

Given that isolate or bind usage fails for Future, it means that they'll fail for any other type that can be waited on. So actually I'm thinking that we should disallow by default code like this, meaning the default implicit should not be available without an import:

Note that it's also related to asynchronicity, IIRC the problem was caused by the timing of Runnable creation which received isolated reference before it was restored to "normal" one. So I think it shouldn't be an issue for Coeval and any other synchronous code.

Other asynchronous code that can't be mapped should be fine as well:

local := 100

context.execute(() => {
  Local.isolate {
    local := 200
    // Will this see 100 or 200?
    printIt()
  }

  // Will this see 100 or 200?
  printIt()
})

In this example, we set local to 100, then we create a new Local reference with value of the original local. it is set to 200 and printed. Print outside of isolated block acts on original reference so it prints 100, as expected.

The problem would occur if a Runnable for second printIt() was created before Local.isolate block would return but that's not possible here.

@alexandru

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

@Avasil I've made a proposal here, as a PR to your repo 🙂

Avasil#1

Avasil added 3 commits Aug 11, 2019
@Avasil

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 11, 2019

Thanks @alexandru

I merged it and added basic tests for Task, Coeval and CompletableFuture. If @oleg-py is fine with current approach, we could go forward and improve it further in later releases.

@Avasil

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 11, 2019

@alexandru after merging with your PR travis always fails with

�[32mmonix.eval.TaskAppSuite�[0m
�[32m- run works�[0m


No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#build-times-out-because-no-output-was-received

The build has been terminated

and I have no idea how to debug it :( have similar thing on cats-effect 2.0 branch

@alexandru

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

Yes, one of the tests is screwed, I noticed it but haven't gotten around to fixing it.

Will take a look tonight.

* Instance for `java.util.concurrent.CompletableFuture`.
*/
implicit def completableFuture[R]: CanBindLocals[CompletableFuture[R]] =
CompletableFutureInstance.asInstanceOf[CanBindLocals[CompletableFuture[R]]]

This comment has been minimized.

Copy link
@oleg-py

oleg-py Aug 12, 2019

Collaborator

Can we do this on 2.11? It's not guaranteed to have Java 8, which is when CompletableFuture was added.

This comment has been minimized.

Copy link
@alexandru

alexandru Aug 12, 2019

Member

Ah, good point. You're right, it's only available starting with Java 8.

@oleg-py

This comment has been minimized.

Copy link
Collaborator

commented Aug 13, 2019

I'm on board, but leave default "any A" instance as a non-implicit, so people can adjust their code if necessary without copy-pasting pieces of Unit instance.

@alexandru

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

I agree, however I'm working on an import Local.Implicits.synchronousAsDefault that does not override the implicits that are already in scope.

Standby please 🙂

@alexandru

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

Hi guys, I've made changes on this PR:

Avasil#2

Can you please take a look?
Thanks.

@Avasil

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 16, 2019

@alexandru I went through it quickly and looks good, I think we could go forward with it

alexandru and others added 2 commits Aug 16, 2019
Merge pull request #2 from alexandru/canbind3
Remove Task/Coeval instances
@Avasil

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 16, 2019

@alexandru implicitbox is not released for 2.13.0-M5 (on the other hand, releasing 2.13.0-M5 has questionable benefit)

@Avasil

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 16, 2019

Oleg also approves so I merged PR to my fork and we could go forward.

I will be travelling tomorrow but I should be available tomorrow in the early evening if we can make a release, I have already prepared release notes.

@alexandru

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

@alexandru implicitbox is not released for 2.13.0-M5 (on the other hand, releasing 2.13.0-M5 has questionable benefit)

It is released for 2.13.0.

Should we release it for 2.13.0-M5?

What's the holdup for us releasing Monix for 2.13 anyway? Cats, Cats-Effect? What's the status there?

@Avasil

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 17, 2019

Yeah, cats supports 2.13 only for 2.0 which is currently in RC. I don't know what are the blockers

@Avasil

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 19, 2019

So how do we proceed? I think the easiest way is to just release implicitbox for 2.13.0-M5 and leave Monix as is

Alternatively we can drop 2.13.0-M5 support and then add 2.13.0 once we can upgrade to Cats-effect 2.0.

@alexandru

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

We should drop 2.13.0-M5. Nobody is nuts enough to use it.

@alexandru

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

What's the holdup on Cats-Effect 2.0 and Scala 2.13 anyway?

@Avasil

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 19, 2019

What's the holdup on Cats-Effect 2.0 and Scala 2.13 anyway?

Cats-Effect 2.0 supports Scala 2.13 in 2.0.0-RC1

A holdup on 2.0 is because of Cats. I think (I'm not sure) scalacheck is no longer blocking them. The problem seems to be this issue: typelevel/cats#2982

@Avasil

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 20, 2019

@alexandru I disabled 2.13.0-M5 for now but there is a failure for 2.12.8 in ScalaJS:
https://travis-ci.org/monix/monix/jobs/573870450

I can't reproduce it locally.

# Executing command line:
java
-Dfile.encoding=UTF8
-Xms1G
-Xmx3G
-XX:ReservedCodeCacheSize=250M
-XX:+TieredCompilation
-XX:-UseGCOverheadLimit
-XX:+CMSClassUnloadingEnabled
-XX:+UseConcMarkSweepGC
-Xmx6144m
-jar
/home/travis/.sbt/launchers/1.2.8/sbt-launch.jar
++2.12.8
ci-js

saving stty: 500:5:bf:8a3b:3:1c:7f:15:4:0:1:0:11:13:1a:0:12:f:17:16:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0
java.lang.NoClassDefFoundError: sjsonnew/BasicJsonProtocol
	at sbt.StandardMain$.exchange$lzycompute(Main.scala:118)
	at sbt.StandardMain$.exchange(Main.scala:118)
	at sbt.StandardMain$.runManaged(Main.scala:140)
	at sbt.xMain.run(Main.scala:89)
	at xsbt.boot.Launch$$anonfun$run$1.apply(Launch.scala:109)
	at xsbt.boot.Launch$.withContextLoader(Launch.scala:128)
	at xsbt.boot.Launch$.run(Launch.scala:109)
	at xsbt.boot.Launch$$anonfun$apply$1.apply(Launch.scala:35)
	at xsbt.boot.Launch$.launch(Launch.scala:117)
	at xsbt.boot.Launch$.apply(Launch.scala:18)
	at xsbt.boot.Boot$.runImpl(Boot.scala:56)
	at xsbt.boot.Boot$.main(Boot.scala:18)
	at xsbt.boot.Boot.main(Boot.scala)
Caused by: java.lang.ClassNotFoundException: sjsonnew.BasicJsonProtocol
	at java.net.URLClassLoader.findClass(URLClassLoader.java:381)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
	... 13 more
Error during sbt execution: java.lang.NoClassDefFoundError: sjsonnew/BasicJsonProtocol
Exception in thread "Thread-0" java.lang.NoClassDefFoundError: sjsonnew/BasicJsonProtocol
	at sbt.StandardMain$.exchange$lzycompute(Main.scala:118)
	at sbt.StandardMain$.exchange(Main.scala:118)
	at sbt.StandardMain$.$anonfun$shutdownHook$1(Main.scala:124)
	at java.lang.Thread.run(Thread.java:748)
Caused by: java.lang.ClassNotFoundException: sjsonnew.BasicJsonProtocol
	at java.net.URLClassLoader.findClass(URLClassLoader.java:381)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
	... 4 more

Any idea?

@oleg-py

This comment has been minimized.

Copy link
Collaborator

commented Aug 20, 2019

@Avasil I've cleared Travis cache for your branch, seems to be running now.

@Avasil

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 20, 2019

Thank you @oleg-py !

@alexandru Can I do a release today? :)

Waiting for Cats-Effect 2.0 could save us small headache in the future but it will take indefinite amount of time and we are on RC since 18 Mar 2018 which is inexcusable even considering our circumstances. :P

@alexandru

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

@Avasil I know you are impatient and so am I. The situation is ridiculous. However:

  1. We have one outstanding incompatibility with Scala 2.11 — Scheduler.features. So we are breaking bin compat with 2.11, but not with 2.12 and this is exactly what Cats-Effect is also doing
  2. Scala 2.13.0 final has been released and people want to play with it. It's regrettable if we don't support it in 3.0.0
  3. We are introducing new features and it's a good practice to have at least one RC that freezes the development of features, before we release the final version

So I'm thinking that if you we want a release today, it should be another RC 🙂 People have been waiting since 18 Mar 2018 for the final version, surely they can wait another week or two.

In the meantime I'll get involved and make sure that Cats 2.0 and Cats-Effect 2.0 are pushed forward.

@Avasil

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 20, 2019

@alexandru
Ok, let's do it this way. Should we release RC for Cats-Effect 2.0-RC1?
And then only upgrade it to 2.0?

@alexandru

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

For Cats-Effect 2.0 RC I would, however Cats 2.0 is actively developed. There's an outstanding issue with Stream vs LazyList. Don't know what will come of it. And the problem is that we'll force the upgrade on all our downstream users.

So no, not yet. If people want Cats-Effect 2.0-RC, they can just force it in their build.

@Avasil

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 20, 2019

Cool, I'm merging this PR then

I'm not able to release today but I can make it tomorrow early. Should we also announce feature freeze so nothing except bug fixes and Cats upgrade will be added in this period?

@alexandru

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

Yes, feature freeze + series/3.0.0 branch, because we can't block master for other contributions.

@Avasil Avasil merged commit d590bd5 into monix:master Aug 20, 2019

1 check passed

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

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

@Avasil btw, I think we still have an issue with this PR that I forgot to fix:
https://github.com/monix/monix/pull/977/files#r312788781

Will make another quick PR tonight.

@Avasil

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 21, 2019

@alexandru It also seems like CompletableFutureLocalSuite failed on master but I'm not sure if the test is correct (I never used it before)

@Avasil Avasil deleted the Avasil:locals-protect-from-error 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.