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

IOLocal - generalize scope function #3360

Merged
merged 2 commits into from
Feb 4, 2023

Conversation

iRevive
Copy link
Contributor

@iRevive iRevive commented Jan 13, 2023

Follow up to #3214 (comment)

@iRevive
Copy link
Contributor Author

iRevive commented Jan 13, 2023

It looks like the test is flaky:

[error] Failed: Total 2494, Failed 3, Errors 0, Passed 2480, Skipped 11
[error] Failed tests:
[error] 	cats.effect.IOAppSpec

@@ -201,15 +201,15 @@ sealed trait IOLocal[A] {
* for {
* local <- IOLocal(42)
* _ <- local.get // returns 42
* _ <- local.scope(0).surround(local.getAndSet(1)) // returns 0
* _ <- local.scope(_ => 0).surround(local.getAndSet(1)) // returns 0
Copy link
Member

Choose a reason for hiding this comment

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

Should we have an example which actually uses the parameter?

Copy link
Contributor Author

@iRevive iRevive Feb 4, 2023

Choose a reason for hiding this comment

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

Sounds good. I updated the example

@iRevive iRevive force-pushed the iolocal-generalize-scope branch 3 times, most recently from 841238a to 6ad666c Compare February 4, 2023 08:11
@djspiewak djspiewak merged commit cc5e9e4 into typelevel:series/3.x Feb 4, 2023
@rossabaker
Copy link
Member

This isn't an MTL Local, but the old scope signature is closer to what is traditionally called scope. I like the new signature, but I'm not so sure about the name.

@armanbilge
Copy link
Member

Hmm good point. It is sort of like a transformAndScope.

Speaking about the signature: is it even safe to expose this as a Resource like this? The problem with doing so is that there is no guarantee that the acquire/and release phases will run on the same fiber.

@rossabaker
Copy link
Member

Have to work a little harder with pure cats-effect, but the old fs2 footgun is still present:

    IOLocal(0).flatMap { local =>
      (Stream.resource(local.scope(1)) >> Stream.eval(local.get))
        .map("Context is "+_)
        .compile
        .lastOrError
    }

"Context is 0" with the map, and 1 without. (It's the hidden interruptScope.)

@armanbilge
Copy link
Member

Ack. Maybe we should go back on this 😕

Breadcrumb to #3385.

@rossabaker
Copy link
Member

To be fair, it does what it says on the tin:

  /*
   * Creates a scope with the given value. The original value is restored upon the finalization
   * of a resource. It means all changes made inside of the resource will not be propagated to
   * the outside.

The resource didn't leak to the outside. We just are surprised to find ourselves on the outside, and had to lift into some foreign effect to do it. It's not the answer for tracing1, but arguably "not CE's problem."

I'd be more upset about this with an example that's:

  1. Pure CE
  2. Doesn't do stupid tricks with .allocated

Footnotes

  1. But might work for otel4s when paired with a Ref.

@armanbilge
Copy link
Member

@rossabaker stupid enough for ya? 😛

//> using lib "org.typelevel::cats-effect::3.4.6"

import cats.effect._
import cats.syntax.all._

object App extends IOApp.Simple:

  extension [A](local: IOLocal[A])
    def scope(value: A): Resource[IO, Unit] =
      Resource.make(local.getAndSet(value))(p => local.set(p)).void

  def run = IOLocal(0).flatMap { local =>
    local.scope(1).race(local.scope(2)).surround(local.get.flatMap(IO.println))
  }

@rossabaker
Copy link
Member

Hmm. Yeah, that's a better footgun than my FS2 example, because it's just IO. But, still, all the method promises is that changes are not visible on the outside, not that they're always visible on the inside!

@iRevive iRevive deleted the iolocal-generalize-scope branch February 5, 2023 07:55
@rossabaker
Copy link
Member

This "fiber ref"'s scope makes Stream behave to my intuition. I'm still contemplating what Arman's example should return. I'm still at the flinging-poo-at-the-wall phase rather than the principled phase.

//> using scala "2.13.10"
//> using lib "org.typelevel::cats-effect:3.4.6"
//> using lib "org.typelevel::cats-mtl:1.3.0"
//> using lib "co.fs2::fs2-core:3.5.0"
//> using plugin "org.typelevel:::kind-projector:0.13.2"

import cats._
import cats.effect._
import cats.effect.syntax.all._
import cats.effect.std._
import cats.mtl._
import cats.syntax.all._
import fs2._

trait FiberRef[F[_], A] {
  def get: F[A]
  def scope(a: A): Resource[F, Unit]
}

object FiberRef {
  def ioFiberRef[A](a: A): IO[FiberRef[IO, A]] =
    for {
      ref <- Ref.of[IO, A](a)
      local <- IOLocal(ref)
    } yield new FiberRef[IO, A] {
      def get: IO[A] =
        local.get.flatMap(_.get)
      def scope(a: A): Resource[IO, Unit] =
        for {
          oldRef <- Resource.eval(local.get)
          newRef <- Resource.eval(oldRef.get.map(Ref.unsafe[IO, A]))
          _ <- Resource.make(local.set(newRef))(_ => local.set(oldRef))
          ref <- Resource.eval(local.get)
          _ <- Resource.make(ref.getAndSet(a))(ref.set)
        } yield ()
    }
}

object Main extends IOApp.Simple {
  implicit class IOOps[A](ioLocal: IOLocal[A]) {
    def scope(value: A): Resource[IO, Unit] =
      Resource.make(ioLocal.getAndSet(value))(p => ioLocal.set(p)).void
  }

  def run =
    demo("basic stream", basicStream) >>
    demo("interrupted sream", interruptedStream) >>
    demo("resources", resources)

  def demo[A](label: String, prog: IO[A]): IO[Unit] =
    prog.flatMap(value => IO.println(s"$label: $value"))

  // prints 1
  def basicStream =
    FiberRef.ioFiberRef(0).flatMap { fiberRef =>
      (Stream.resource(fiberRef.scope(1)) >> Stream.eval(fiberRef.get))
        .compile
        .lastOrError
    }

  // prints 1
  def interruptedStream =
    FiberRef.ioFiberRef(0).flatMap { fiberRef =>
      (Stream.resource(fiberRef.scope(1)) >> Stream.eval(fiberRef.get))
        .map(identity)
        .compile
        .lastOrError
    }

  // prints 0
  def resources =
    FiberRef.ioFiberRef(0).flatMap { fiberRef =>
      fiberRef.scope(1).race(fiberRef.scope(2)).surround(fiberRef.get)
    }
}

@armanbilge
Copy link
Member

If I may fling some 💩 too: consider also the both variant:

local.scope(1).both(local.scope(2)).surround(local.get.flatMap(IO.println))

I can't see how that's supposed to do something sensible at all :)

@rossabaker
Copy link
Member

That's a good example. For the local.get to be "inside" the both resource, the environment would need a semigroup.

I think I'm okay with the resource behavior. We're not using local.scope(1) nor local.scope(2) directly. We're using a new, compound resource that isn't scoped, and therefore has the 0 environment. In tracing terms, our resource might link to 1 or 2, but I don't believe it's spanned by either.

Now to reconcile this with my spittle at what happens in FS2 without extraordinary Ref efforts...

@armanbilge
Copy link
Member

Yeah, I wasn't so okay with it in the end 😕 I opened a PR to remove this method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants