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

Incorrect bracketState implementation #841

Open
ioaoue opened this issue Dec 9, 2021 · 1 comment
Open

Incorrect bracketState implementation #841

ioaoue opened this issue Dec 9, 2021 · 1 comment

Comments

@ioaoue
Copy link
Collaborator

ioaoue commented Dec 9, 2021

The current implementation of bracketState does not guarantee that:

  • use will be cancelled when fiber is cancelled
  • commit will be executed if fa succeeds

The following code demonstrates the problem:

import cats.effect._
import java.time.Instant
import scala.concurrent.duration._
import tofu.syntax.bracket._

object Demo extends IOApp {
  def log(text: String): IO[Unit] = IO(println(s"${Instant.now}: $text"))

  val slow: IO[Unit] = for {
    _ <- log("start")
    _ <- Timer[IO].sleep(3.seconds)
    _ <- log("finish")
  } yield ()
  
  override def run(args: List[String]): IO[ExitCode] = 
    slow.bracketState { _ => 
      log("use").as((), ())
    } { _ =>
      log("commit")
    }.timeoutTo(1.second, log("timeout"))
      .as(ExitCode.Success)
}

The output looks like this:

2021-12-09T17:18:30.869187693Z: start
2021-12-09T17:18:33.897543168Z: finish
2021-12-09T17:18:33.899531757Z: use
2021-12-09T17:18:33.900238459Z: timeout

The "finish" message is still printed although the fiber is cancelled by timeoutTo. This is because the first FG.bracket call in bracketState code makes its init block uncancellable. The "commit" message is not printed because bracketCase calls it in the use block of the first FG.bracket, but it is not guaranteed that use will be executed after successful init.

This behavior breaks bracketModify and MVar-based tofu.memo.Cached.

@Odomontois
Copy link
Member

Odomontois commented Dec 15, 2021

Yeah, that one is tricky.
This can be easily achieved with ZIO and CE3 but can be tricky in CE2 since it doesn't privide use result for the finalizer the bracket signature

My thoughts on how and when we fix this.
This PR moves tofu.concurrent.Exit to the kernel. After that that way we can use Finally[F, tofu.concurrent.Exit] as a common interface for all three. Next, we can use it in the fixed bracketState version implementing it as a single bracket call.
We need to implement this common Finally thing for all three effects. CE2 version would have some dirty Ref hack to store use results, but it should do the trick.

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

No branches or pull requests

2 participants