Resource#attempt holds onto acquired resources even in case of error

[armanbilge]

//> using dep org.typelevel::cats-effect::3.5.1

import cats.effect.*
import cats.syntax.all.*

object Bug extends IOApp.Simple:
  def run = IO.ref(0).flatMap { ref =>
    val resource = Resource.make(ref.update(_ + 1))(_ => ref.update(_ + 1))
    val error = Resource.raiseError[IO, Unit, Throwable](new Exception)

    val attempt = (resource *> error).attempt

    attempt.use { r =>
      ref.get.flatMap { i =>
        IO {
          // acquiring the resource as a whole failed, so it should be released in entirety
          assert(r.isLeft, r.toString)
          assert(i == 2, i.toString) // assertion failed: 1
        }
      }
    }
  }

[durban]

Hm, if it failed, why is the body of use called at all? But, I don't think it failed: it succeeded with Left(new Exception).

[armanbilge]

Yes you are exactly right. However although the resource acquisition failed (prior to the attempt) it has not run all the finalizers yet.

[durban]

So, you're basically saying, that since resource *> error failed, it should run the finalizer of resource even before continuing to the .attempt, right? I guess that makes sense, (even if it seems a little strange to me)...

[armanbilge]

even if it seems a little strange to me

why does that feel strange? I'm drawing a distinction between (resource *> error).attempt and resource *> error.attempt. Currently they behave the same.

[durban]

Yes, exactly: it seems strange to me that those two would be different. (To be clear, we're obviously assuming resource does not fail here.) Based on this comment by @djspiewak, it seems I might not be alone. At least I think that law would imply that they should be the same...

[armanbilge]

To be clear, we're obviously assuming resource does not fail here

👍

At least I think that law would imply that they should be the same...

Hmm, yes, I think I interpret that law the same way. Thanks for the pointer, let me think about this a bit 🤔

[armanbilge]

Since I haven't mentioned it yet, the problem with the current semantics is that if you try to implement some sort of retry then failed resources don't get released in a timely fashion.

For example, here's a rough sketch of a connection pool as used in Ember client or Skunk.

def getSocketFromPool: Resource[IO, Socket[IO]] = ???

def raiseIfConnectionIsDead(socket: Socket[IO]): IO[Unit] = ???

def getAliveSocketFromPool =
  getSocketFromPool.evalTap(raiseIfConnectionIsDead).attempt.flatMap {
    case Right(socket) => Resource.pure(socket)
    case Left(DeadConnection) => getAliveSocketFromPool // retry to obtain a fresh connection
    case Left(ex) => Resource.raiseError(ex) // raise all other errors
  }

Under the current semantics, every time getAliveSocketFromPool loops after the attempt it will keep requesting a new socket from the pool without returning the dead one (so that the pool can replace it). This is problematic because in practice there are limits (e.g. the number of open file descriptors or connections in the pool per-host) so the retry may get stuck because each iteration only takes from the pool and does not return anything back to the pool.

A similar bug plagued us in http4s/http4s#7216 (comment), although note Resource#attempt was not being used there.

[SystemFw]

Format is ugly, but this is relevant context:

https://discord.com/channels/632277896739946517/839263556754472990/1141812346877132860

SystemFw — Today at 21:14
maybe I'm weird, and I get that retry will be a pain, but it behaves like I expect it

armanbilge — Today at 21:14
really?
daniel — Today at 21:15
that's absolutely fascinating

armanbilge — Today at 21:15
the fact that its holding onto resources that you can no longer access seems really wrong to me

SystemFw — Today at 21:15
it's not holding onto them though

daniel — Today at 21:15
so fwiw, this is a major way in which fa.attempt *> fb is different from fa !> fb

armanbilge — Today at 21:15
by "holding on" I mean "not finalizing"

SystemFw — Today at 21:15
meaning, Resource has no concept of "no longer have access"
that's defined by exiting it, with use

armanbilge — Today at 21:16
I forget what !> is

SystemFw — Today at 21:16
as far as Resource is concerned, all resources are open
except for !>, which closes a scope

daniel — Today at 21:16
it's forceR. a bit like productR except it discards all errors recursively on the left and can behave asymmetrically with ap, and Resource interprets it to close scopes
that scope-closing behavior isn't arbitrary either, iirc

armanbilge — Today at 21:17
hmmm

daniel — Today at 21:17
like I think it's wound up with MonadCancel semantics in some odd way that I can't recall
so basically retry is fine as long as you use !> instead of trying to .attempt your way to victory

armanbilge — Today at 21:17
but with !> you can't pass the result value

SystemFw — Today at 21:18
let me put it this way: there is no early finalisation in Resource
it boils down to that

daniel — Today at 21:18
(apart from !>)
so basically for retry, you have to run your step and then see if you got errors
if you didn't get errors, lovely! return
if you did get errors, forceR the recursion

armanbilge — Today at 21:19
interesting

daniel — Today at 21:19
it's that recursive step that you need to cover. instead of using *> there, use !>

armanbilge — Today at 21:19
yeah I see, hmm

SystemFw — Today at 21:19
iirc we were discussing Stream.++ and scoping
and this popped up

daniel — Today at 21:19
I love the ++ vs >> dichotomy on this one

armanbilge — Today at 21:19
oooh

daniel — Today at 21:19
with Stream it's a bit more intuitive though because you have flatMap vs append

armanbilge — Today at 21:20
yeah

daniel — Today at 21:20
Resource is harder because it's all flatMap

armanbilge — Today at 21:20
I need to thonk it but I can see the parallel
yeah, yeah, right
that helps

daniel — Today at 21:20
!> is a bit like .drain ++

SystemFw — Today at 21:20
it's even closer if you also think about Stream and Pull

daniel — Today at 21:20
yep

SystemFw — Today at 21:20
++ is Pull.flatMap

armanbilge — Today at 21:20
I'm not fully convinced this will help
but I need to try it

SystemFw — Today at 21:20
but Stream has to discard the Pull output to encode things
and similarly, you have !> but not !>>=

daniel — Today at 21:21
the day I realized this was one of the happiest Scala-related days I've ever had. the duality is so obvious and so smart
Pull is just stupidly clever

SystemFw — Today at 21:21
and how we have to copy all this to github 🙂 😦

daniel — Today at 21:21
lmao
yeah this is all very important context

[armanbilge]

Yes, exactly: it seems strange to me that those two would be different. (To be clear, we're obviously assuming resource does not fail here.) Based on this comment by @djspiewak, it seems I might not be alone. At least I think that law would imply that they should be the same...

Just playing around, seems this law is not held for other transformer implementations e.g. WriterT.

//> using dep org.typelevel::cats-core::2.10.0
//> using option -Ykind-projector:underscores

import cats.*
import cats.data.*
import cats.syntax.all.*

@main def main =
  val write = WriterT.tell[Option, String]("hello")
  val error = (()).raiseError[WriterT[Option, String, _], Unit]

  println((write *> error).attempt.run)
  println((write *> error.attempt).run)

Some((,Left(())))
Some((hello,Left(())))

[lenguyenthanh]

I'm curious, what is the conclusion of this? Following the discussion here, it is lawful and even painful but nothing We can do. But in http4s/http4s#7445, @armanbilge said We should fix it for 3.6.0 🤔

Edit: fixed typo

[armanbilge]

@lenguyenthanh thanks for following up. @djspiewak and I discussed this more at some point and I think that Daniel came around and supports changing the behavior to fix this.

Following the discussion here, it is lawful and even painful but nothing We can do.

When we talk about lawfulness, we need some notion of equivalence between two effectual computations. If I remember correctly, what Daniel and I discussed was how to consider Resource's finalizers when determining equivalence. In this case, if you don't consider when the finalizer runs, then we are not violating lawfulness by changing attempt as I proposed here.

This would not be the first time that we adjusted when Resource's finalizers run, also needed to change memoize.

• Override memoize for Resource #3080.

[lenguyenthanh]

thanks @armanbilge for the explanation! I'll try to see what I can do.

[lenguyenthanh]

I've been looking at the code, but up until now I couldn't find a solution for this.

Look at this code:

cats-effect/kernel/shared/src/main/scala/cats/effect/kernel/Resource.scala

Lines 705 to 711 in 7168625

	case Allocate(resource) =>
	Resource.applyFull { poll =>
	resource(poll).attempt.map {
	case Left(error) => (Left(error), (_: ExitCase) => F.unit)
	case Right((a, release)) => (Right(a), release)
	}
	}

To fix this issue, We need to execute the finalizer in the left case. But with the attempt, We don't have access to the finalizer, so We need to do something else like Resource.allocatedCase, but then F has to be a MonadCancel, which We don't have here.

I probably missing something here, I don't know how to proceed. Any pointer is much appreciated.

[armanbilge]

@lenguyenthanh that's all exactly right actually 💯

I probably missing something here, I don't know how to proceed.

You can go ahead and add the MonadCancel constraint (in fact, I think it has to be MonadCancelThrow). Meanwhile, we will keep the old version of the method and deprecate it, similar to the onError PR.

[lenguyenthanh]

@armanbilge here is my naive implementation, couldn't make it work with just attempt because the ambiguity.
Could you please give some more help 🙏

  // Can't use `attempt` function here as it would cause ambiguity with the other `attempt`
  def safeAttempt(implicit F: Concurrent[F]): Resource[F, Either[Throwable, A]] = {
    // We need a Ref[Option[Resource.ExitCase => F[Unit]]] because We want to release
    // all on hold resources in case of error
    Resource.eval(F.ref(none[ExitCase => F[Unit]]).flatMap { release =>
      this
        .allocatedCase
        .flatMap { case (a, r) => release.update(_ => r.some).as(a) }
        .attempt
        .flatMap {
          case Left(error) =>
            release.get.flatMap(_.traverse_(_(ExitCase.Errored(error)))).as(error.asLeft[A])
          case Right(a) => a.asRight[Throwable].pure[F]
        }
    })
  }