Potential inconsistency between joinWithNever and docs

[wunderk1nd-e]

trafficstars

Hi there,

In the docs for the the Spawn typeclass at the very end it states:

In English, the semantics of this are as follows:

If the child fiber completed successfully, produce its result If it errored, re-raise the error within the current fiber If it canceled, attempt to self-cancel, and if the self-cancelation fails, deadlock Sometimes this is an appropriate semantic, and the cautiously-verbose joinWithNever function implements it for you.

However, the joinWithNever implementation doesn't look like it actually makes any attempts at self cancellation and I think I've confirmed this with a small test app.

object FunTest extends IOApp.Simple {
  override val run: IO[Unit] = for {
    _ <- IO.println("Starting")
    child = IO.println("Starting child...") >>
      IO.sleep(1.second) >>
      IO.canceled >>
      IO.sleep(1.second) >>
      IO.println("Child completed")
    childFiber     <- child.start
    dependentChild <- childFiber.joinWithNever.as("I managed to return something").start
    result <- dependentChild
      .joinWith(IO.pure("I was cancelled"))
      .timeoutTo(3.seconds, "I waited too long :(".pure[IO])
    _ <- IO.println(s"result is $result")
  } yield ()
}

which outputs the following to console

Starting
Starting child...
result is I waited too long :(

Process finished with exit code 0

[durban]

Yeah, that's definitely a bug; either in the code or in the docs.

[armanbilge]

I'm reminded of a similar confusion with embedNever which we discussed in https://github.com/typelevel/cats-effect/pull/3351#discussion_r1070651800.

[lenguyenthanh]

yeah, tbh I wasn't sure which way I should go fix the docs or fix the code. My intuition is joinWithNever seems safer as the doc says then the current implementation. I'm happy to fix the doc too if We want to keep the current semantic of joinWithNever.

[armanbilge]

@lenguyenthanh your PR passes our test suite so that's a good sign that we are not depending on the bug and hopefully nobody else is either. Also we have precedent for fixing bugs even if they did cause some breakage ... (see the async_ bugfix in 3.5), not saying we want to repeat that necessarily, just saying we did do it 😅

[lenguyenthanh]

I agree that We need to be careful about the changes. So, the question is what does it take to introduce this change? Or should we be safe and update the docs instead?

[lenguyenthanh]

As we prefer updating the doc (#4131), how about update the docs like this (just copy/paste from scala doc)?:

-- If it canceled, attempt to self-cancel, and if the self-cancelation fails, **deadlock**
+- If it canceled, the caller is indefinitely suspended without termination (a.k.a **deadlock**)

[durban]

Was fixed by #4160.