Add MVar #217

alexandru · 2018-05-14T12:31:50Z

Adds MVar to Cats-Effect.

PR also contains website documentation.

Implementation and documentation are imported from Monix, with some changes for working with F[_] and for cancelability.

SystemFw · 2018-05-14T12:38:01Z

Only had a very quick glance, but one thing: you need read to be a primitive operation.
Having read be take then put leads to all sort of weird behaviour (mostly deadlock) due to concurrent interleaving (e.g. someone puts in between your take and your put).

The original MVar in Haskell had this issue, and read was changed to be primitive because of that.

The other question is adding derived operations, specifically which ones should be added (e.g. modify, modifyF or none) and related questions/docs about their nature (again, mostly due to them not being atomic).

codecov-io · 2018-05-14T12:38:33Z

Codecov Report

Merging #217 into master will increase coverage by 0.01%.
The diff coverage is 87.68%.

@@            Coverage Diff             @@
##           master     #217      +/-   ##
==========================================
+ Coverage   88.64%   88.66%   +0.01%     
==========================================
  Files          54       57       +3     
  Lines        1251     1385     +134     
  Branches      100      135      +35     
==========================================
+ Hits         1109     1228     +119     
- Misses        142      157      +15

alexandru · 2018-05-14T12:54:12Z

@SystemFw

Only had a very quick glance, but one thing: you need read to be a primitive operation.
Having read be take then put leads to all sort of weird behaviour (mostly deadlock) due to concurrent interleaving (e.g. someone puts in between your take and your put).

This is why I left read out of it. If you're designing MVar like a concurrent queue, which it is, read is difficult to introduce as an atomic operation.

Are there any MVar implementations that provide an atomic read? And is it worth the complexity cost?

The other question is adding derived operations, specifically which ones should be added (e.g. modify, modifyF or none) and related questions/docs about their nature (again, mostly due to them not being atomic).

I would lean towards not including any derived operations.

SystemFw · 2018-05-14T12:59:02Z

Are there any MVar implementations that provide an atomic read? And is it worth the complexity cost?

Well, the MVar implementation (the one in GHC) does, and it was deemed to be worth the cost due to the many problems implementing things that require reading.

If you're designing MVar like a concurrent queue, which it is, read is difficult to introduce as an atomic operation.

I think this is an implementation detail that shouldn't influence the interface one way or another.

I would lean towards not including any derived operations.

No strong opinions here, I can see pros and drawbacks of both

alexandru · 2018-05-14T13:01:23Z

@SystemFw OK, will try to add it

alexandru · 2018-05-14T13:03:00Z

@SystemFw so I did not remember read from MVar being atomic, but apparently this changed in "base 4.7": https://hackage.haskell.org/package/base-4.9.1.0/docs/Control-Concurrent-MVar.html#v:readMVar

SystemFw · 2018-05-14T13:03:54Z

so I did not remember read from MVar being atomic, but apparently this changed in "base 4.7"

Yeah that's what I meant, it proved too inconvenient/error prone in practice, and it was changed 👍

alexandru · 2018-05-14T16:16:26Z

@SystemFw pushed a commit with an implementation that supports the atomic read.

I have to add more tests and maybe clean it up a bit.

SystemFw · 2018-05-14T19:08:20Z

core/shared/src/main/scala/cats/effect/concurrent/MVar.scala

+ *  - [[take]] which empties the var if full, returning the contained
+ *    value, or blocks (asynchronously) otherwise until there is
+ *    a value to pull
+ *


read needs to be added here

alexandru · 2018-05-14T19:36:33Z

I've added tests and documentation. It is now ready for review.

johnynek · 2018-05-14T19:52:11Z

core/shared/src/main/scala/cats/effect/internals/Callback.scala

@@ -41,10 +41,10 @@ private[effect] object Callback {
    }

  /** Reusable `Right(())` reference. */
-  final val rightUnit = Right(())


why are these not final now?

AFAIK final val in companion objects will make the compiler treat that as a constant and inline it. Which would defeat the point of declaring it in the first place, as we're doing such shenanigans in order to not allocate memory every time we need a Right(()).

@non I thought told me that was only if there is no type annotation, so we could add that but keep the final. But in any case probably not a big deal.

Yes, final val x = ... will inline but final val x: SomeType = ... will not. It's a weird rule.

Is there any difference between final val x: SomeType = ... and val x: SomeType?

From the JVM's point of view both will be final, so I'd rather go with the later.

johnynek · 2018-05-14T19:53:02Z

core/shared/src/main/scala/cats/effect/internals/LinkedMap.scala

+    emptyRef.asInstanceOf[LinkedMap[K, V]]
+
+  private val emptyRef =
+    new LinkedMap[Any, Any](Map.empty, LongMap.empty, 0)


strictly speaking I think Nothing would be a better type for the value.

johnynek · 2018-05-14T19:54:43Z

core/shared/src/main/scala/cats/effect/internals/MVarAsync.scala

+  private def unsafePut(a: A)(onPut: Listener[Unit]): Unit = {
+    if (a == null) {
+      onPut(Left(new NullPointerException("null not supported in MVarAsync/MVar")))
+      return


rather than using return could we just wrap the rest in an else?

Reviewers have to always think "is this a return that is actually ok or not?" if we don't

johnynek · 2018-05-14T19:55:28Z

core/shared/src/main/scala/cats/effect/internals/MVarAsync.scala

+      case current @ WaitForTake(value, puts) =>
+        val update = WaitForTake(value, puts.enqueue(a -> onPut))
+        if (!stateRef.compareAndSet(current, update)) {
+          // $COVERAGE-OFF$


do we need to do this? can we not try to trigger races that will actually hit this branch?

Triggering race conditions that hit those branches is very difficult and you're not gaining much.

It makes me nervous if we have concurrency primatives we aren’t testing in race conditions.

Seems like it shouldn’t be too hard to hit he branch: just have N threads and maybe two MVars that they take from one and put to the other or something like that.

That's easier said than done. In Monix I've got plenty of tests like that and I hate writing them with all my heart 🙂 Note that the current tests are concurrent, but hitting all of those branches is hard an non-deterministic.

That said, I just removed the coverage annotations. So if others want to improve code coverage, they are free to do it.

oleg-py · 2018-05-15T06:37:05Z

core/shared/src/main/scala/cats/effect/concurrent/MVar.scala

+      F.delay(MVarAsync[F, A](a)(F))
+
+    /** Builds an empty `MVar`. */
+    def empty[A](implicit F: Async[F]): F[MVar[F, A]] =


Why do you have a second Async here? You already have one as a class parameter.

Good catch. It's from my experiments 🙂

gvolpe · 2018-05-15T07:25:22Z

core/shared/src/main/scala/cats/effect/concurrent/MVar.scala

+   *
+   * @see [[async]]
+   */
+  def apply[F[_]](implicit F: Concurrent[F]): ConcurrentBuilder[F] =


TBH I would prefer to have a simple def apply[F[_]: Concurrent): MVar[F, A]. I understand it brings some type inference help to scalac but on the other hand is less clearer to read and it is not consistent with the other datatypes recently introduced (Ref, Deferred, Semaphore and Resource).

gvolpe · 2018-05-15T07:29:49Z

site/src/main/tut/datatypes/mvar.md

+  until there is a value available, at which point the operation resorts
+  to a `take` followed by a `put`
+
+An additional but non-atomic operation is `read`, which tries reading the


read is now an atomic operation? :)

gvolpe · 2018-05-15T07:31:52Z

site/src/main/tut/datatypes/mvar.md

+}
+```
+
+## Use-case: Producer/Consumer Channel


Great example~ 👍

alexandru · 2018-05-15T12:16:38Z

@johnynek so regarding your review, I've added another concurrency test with take, put and read operations racing each other. I also removed the coverage annotations, as mentioned before.

@gvolpe

TBH I would prefer to have a simple def apply[F[_]: Concurrent): MVar[F, A]. I understand it brings some type inference help to scalac but on the other hand is less clearer to read and it is not consistent with the other datatypes recently introduced (Ref, Deferred, Semaphore and Resource).

In my experience thus far with Cats's monad transformers and with Monix's Iterant, without partially applying types in apply, these data types parameterized for F[_] become really annoying. As for the others:

this isn't new and has been done in Cats, EitherT as mentioned for example
Ref is addressed in Fix #210: curry type params of Ref constructors #211
Deferred doesn't need it because you aren't giving it an initial value, so there's no type inference to be had for it
Semaphore doesn't need it, because it doesn't have an A type parameter
Resource doesn't need it because apply takes an F[_] as parameter and so the Scala compiler is perfectly fine inferring the types for that

Besides Ref, none of the other data types mentioned are relevant.

I did make changes. So in addition to apply's magic, I added those builders as plain functions as well. So as mentioned in the ScalaDoc:

MVar[IO].empty[Int] <-> MVar.empty[IO, Int]

MVar[IO].init("hello") <-> MVar.init[IO, String]("hello")

MVar[IO].initF(IO("hello")) <-> MVar.initF(IO("Hello"))

Also there's no more async, so users will have to use the normal emptyAsync, initAsync or initAsyncF.

mpilquist · 2018-05-15T13:54:53Z

core/shared/src/main/scala/cats/effect/concurrent/MVar.scala

+   *
+   * @see [[init]], [[initF]] and [[empty]]
+   */
+  def apply[F[_]](implicit F: Concurrent[F]): ApplyBuilders[F] =


I like this pattern - can we use it in Ref too? Specifically the notion of having both the regular method on companion and then ApplyBuilders for the type curried type params.

rossabaker

👍

- Ref.ApplyBuilders is introduced and made public, similar to typelevel#217 - Refs can be constructed as Ref[F, A].of(a) or Ref[F].of(a) - Unsafe builder is only available as a version with all type parameters

Initial MVar

177f73f

Fix builders

0ee507c

Atomic read

5ec0706

Add tests

26b8c61

SystemFw reviewed May 14, 2018

View reviewed changes

Add documentation

3b6d8d7

alexandru changed the title ~~WIP: MVar~~ Add MVar May 14, 2018

Relax tests

88fc481

johnynek reviewed May 14, 2018

View reviewed changes

alexandru added 5 commits May 15, 2018 08:52

Remove null check

5a91dcf

LinkedMap[Nothing, Nothing] instead of LinkedMap[Any, Any]

7cd538a

Refactoring

a6dfa4b

Fix TimerTest

140458d

MVar - remove coverage annotations

87d577b

oleg-py reviewed May 15, 2018

View reviewed changes

alexandru added 2 commits May 15, 2018 09:47

Fix comments

e27c707

Remove Async param

8837970

gvolpe reviewed May 15, 2018

View reviewed changes

site/src/main/tut/datatypes/mvar.md

}

```

## Use-case: Producer/Consumer Channel

Copy link

Member

gvolpe May 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great example~ 👍

Refined builders, added test

5c33198

alexandru added 3 commits May 15, 2018 14:04

Change MVar builders

e61bf96

Fix doc

a027384

Make test more complicated

18d3161

mpilquist reviewed May 15, 2018

View reviewed changes

mpilquist self-requested a review May 15, 2018 14:01

mpilquist approved these changes May 15, 2018

View reviewed changes

rossabaker approved these changes May 15, 2018

View reviewed changes

ChristopherDavenport approved these changes May 15, 2018

View reviewed changes

SystemFw merged commit 70d58a0 into typelevel:master May 15, 2018

oleg-py mentioned this pull request May 15, 2018

Ref.of builder #220

Merged

alexandru added this to the 1.0.0-RC2 milestone May 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MVar #217

Add MVar #217

alexandru commented May 14, 2018 •

edited

SystemFw commented May 14, 2018

codecov-io commented May 14, 2018 •

edited

alexandru commented May 14, 2018

SystemFw commented May 14, 2018

alexandru commented May 14, 2018

alexandru commented May 14, 2018

SystemFw commented May 14, 2018

alexandru commented May 14, 2018

SystemFw May 14, 2018

alexandru commented May 14, 2018

johnynek May 14, 2018

alexandru May 14, 2018

johnynek May 15, 2018

non May 15, 2018

alexandru May 15, 2018

johnynek May 14, 2018

johnynek May 14, 2018

johnynek May 14, 2018

alexandru May 14, 2018

johnynek May 15, 2018

alexandru May 15, 2018

oleg-py May 15, 2018

alexandru May 15, 2018

gvolpe May 15, 2018

gvolpe May 15, 2018

gvolpe May 15, 2018

alexandru commented May 15, 2018 •

edited

mpilquist May 15, 2018

rossabaker left a comment

Add MVar #217

Add MVar #217

Conversation

alexandru commented May 14, 2018 • edited

SystemFw commented May 14, 2018

codecov-io commented May 14, 2018 • edited

Codecov Report

alexandru commented May 14, 2018

SystemFw commented May 14, 2018

alexandru commented May 14, 2018

alexandru commented May 14, 2018

SystemFw commented May 14, 2018

alexandru commented May 14, 2018

Choose a reason for hiding this comment

alexandru commented May 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexandru commented May 15, 2018 • edited

Choose a reason for hiding this comment

rossabaker left a comment

Choose a reason for hiding this comment

alexandru commented May 14, 2018 •

edited

codecov-io commented May 14, 2018 •

edited

alexandru commented May 15, 2018 •

edited