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

Add WeakAsync.liftIO #1910

Merged
merged 1 commit into from
Sep 5, 2023
Merged

Conversation

armanbilge
Copy link
Member

So that we can specifically get a LiftIO. h/t @henricook for the idea.

@mergify mergify bot added the free label Sep 5, 2023
@jatcwang jatcwang merged commit eca423e into typelevel:main Sep 5, 2023
5 checks passed
@henricook
Copy link

@armanbilge Thanks again for this. Could I beg a quick sanity check from you please?

My placeholder for this was:

implicit class RichIOLiftCIO[B](val value: IO[B]) extends AnyVal {
    def liftToConnectionIO: ConnectionIO[B] = {
      Sync[ConnectionIO].delay(value.unsafeRunSync()(IORuntime.global))
    }
  }
}

and now I have:

implicit class RichIOLiftCIO[B](val value: IO[B]) extends AnyVal {
    def liftToConnectionIO: ConnectionIO[B] = {
      WeakAsync.liftIO[ConnectionIO].map(_.liftIO(value)).allocated.map(_._1).unsafeRunSync()(IORuntime.global)
    }
  }
}

I think this is basically equivalent right? And I'm unsafeRunSync'ing because the whole point of lifting this, for me, is to roll an IO result into a CIO for comprehension

@henricook
Copy link

henricook commented Nov 20, 2023

This example, above, doesn't actually appear to work 😂 👀

@armanbilge
Copy link
Member Author

armanbilge commented Nov 20, 2023

@henricook I'm not sure why it's not working, but there's a lot of non-idiomatic things happening there.

Can you build the WeakAsync.liftIO[ConnectionIO] first thing in your IOApp, and then just pass it around implicitly e.g.

object App extends IOApp.Simple {
  def run = WeakAsync.liftIO[ConnectionIO].use { implicit lifter =>
    // ...
    ioB.to[ConnectionIO] // this is a method on IO, you don't need your own syntax
    // ...
  }
}

@henricook
Copy link

henricook commented Nov 20, 2023

Wow that's one of those things where I was totally blind to it until you said that, and definitely shouldn't have been.

On the whole putting this in my IOApp and passing it around implicitly works. One area where I have trouble is in my effectful logging wrapper:

trait Logging {
  lazy val dbLogger: MyLogger[ConnectionIO] = loggerF[ConnectionIO]

  private lazy val _logger: Logger = LoggerFactory.getLogger(getClass.getName) // slf4j
  
  @nowarn def loggerF[F[_]: LiftIO: Sync: Functor]: MyLogger[F] = MyLogger(_logger)
}

Because this trait's just mixed in anywhere I need logging, I don't think that I have a way to pass in the LiftIO injected by IOApp. Without passing it to almost every service/controller/connector in the app anyway. So that's my only remaining thinker 🤔 .

I appreciate that one's very much a my app problem and no longer a general usability problem. Thanks for the education above ^!

@henricook
Copy link

henricook commented Nov 20, 2023

Why does a Logger need to lift IO to CIO? Because at some points it reads 'Audit Context' (request ids etc.) to include in the log entries from an IOLocal - which is another topic I know you love to discuss :-D

@henricook
Copy link

henricook commented Nov 20, 2023

Even with everything wired up to use the IOApp injected resource (excluding the Logger which i've just no-op'd for now):

  // Rich IO / ConnectionIO
  implicit class RichIOLiftCIO[B](val value: IO[B]) extends AnyVal {

    def liftToConnectionIO(implicit lifter: LiftIO[ConnectionIO]): ConnectionIO[B] = {
      value.to[ConnectionIO]
    }
  }

Despite compiling fine, it's like the action never gets evaluated so acceptance tests that do these actions aren't seeing the expected results.

Items of potential interest, I'm in a Play Application so building an ApplicationModule with an ApplicationLoader, like this:

   for {
      ....
      (liftCIO, liftCIOStopTasks)                             <- WeakAsync.liftIO[ConnectionIO].allocated
    } yield {
      val myAppModule = new ApplicationModule(
        context,
        dataSourceWithStats,
        database,
        liftCIO,
        ...
        )

In the ... here I'm building http4s clients and other things that use Resource that all work fine.

I deal with Resources this way in Play and tie the shutdown/dispose actions into Play's 'onShutDown' hooks

@armanbilge
Copy link
Member Author

Sorry, I'm not sure why it's not working for you 😕 if you can get it down to a runnable example I can take a look.

@henricook
Copy link

henricook commented Nov 20, 2023

FWIW I could not recreate the issue with:

package com.example

import cats.effect.{IO, IOApp, LiftIO}
import doobie.{ConnectionIO, FC, Transactor, WeakAsync}
import doobie.implicits._
import doobie.util.transactor.Strategy
import org.mockito.ArgumentMatchers.any
import org.mockito.MockitoSugar

import java.sql.Connection

object Main extends IOApp.Simple with MockitoSugar {

  // Create a mock transactor with Mockito
  lazy val dummyRecoveringTransactor: Transactor[IO] = {
    val mockConnection = mock[Connection]
    doNothing.when(mockConnection).setClientInfo(any[String], any[String])
    doNothing.when(mockConnection).setTransactionIsolation(any[Int])

    val s: Strategy = doobie.util.transactor.Strategy(FC.unit, FC.unit, FC.unit, FC.unit)

      Transactor
        .fromConnection[IO](mockConnection, logHandler = None)
        .copy(strategy0 = s)
  }

  def run: IO[Unit] = WeakAsync.liftIO[ConnectionIO].use(x => printWithLift(x).transact(dummyRecoveringTransactor))

  def printWithLift(liftCIO: LiftIO[ConnectionIO]): ConnectionIO[Unit] = for {
    _ <- WeakAsync[ConnectionIO].pure(println("Hi there!"))
    - <- liftCIO.liftIO(IO(println("Hi I'm in the middle!")))
    _ <- WeakAsync[ConnectionIO].pure(println("Bye there!"))
  } yield ()
}

Prints:

Hi there!
Hi I'm in the middle!
Bye there!

@henricook
Copy link

Oh but here we are, changing that run line to: def run: IO[Unit] = WeakAsync.liftIO[ConnectionIO].allocated.map(_._1).map(x => printWithLift(x).transact(dummyRecoveringTransactor)) and all you get is:

Hi there!

Published here: https://github.com/henricook/doobie-liftio-repro1123/blob/main/src/main/scala/com/example/Main.scala

It could just be that I've done something foolish in the quick project setup

@armanbilge
Copy link
Member Author

armanbilge commented Nov 20, 2023

@henricook I believe that should be flatMap(x => printWithLift(...)

@henricook
Copy link

Right you are, curses!

I think I've taken up enough of your time and my old bodge is holding, so I'll try to ignore the problem until one day inspiration strikes. Thanks again for your work and help here.

@henricook
Copy link

henricook commented Nov 20, 2023

Ah, the problem was not my implementation of lifting but the fact that this lift method nukes IOLocal (with your cats effect branch's unsafe changes in effect Arman) and my original did not. Probably I guess (total guess tbh) because of the boundary created through the use of a Dispatcher?

More background: Lots of my code that actions changes also must audit changes for them to succeed. Audit was failing because the AuditContext (loaded from IOLocal) was missing with the new lifting method.

@armanbilge
Copy link
Member Author

armanbilge commented Nov 20, 2023

Ah yes, we discussed this. I don't have a good idea yet for how to get IOLocals to propagate through Dispatchers.

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

Successfully merging this pull request may close these issues.

3 participants