`IO.readLine.timeoutTo(...)` not cancelling the `readLine` after timeout.

[VzxPLnHqr]

trafficstars

This might be related to the decision made here regarding readLine https://github.com/typelevel/cats-effect/issues/1793

In the following simple program, the expected behavior (I think?) is that it should still terminate after the timeout, since a default value has been supplied. However, instead it seems to hang waiting for something to be input by the user (only to ignore it later). I am not sure if this is a bug or if this is the expected behavior?

object Main extends IOApp.Simple {
    val run = for {
        _ <- IO.println("10 seconds to enter something:")
        v <- IO.readLine.timeoutTo(10.seconds,IO("a default value"))
        _ <- IO.println(s"you said $v")
    } yield ()
}

It seems like it is may be a bug since readLine is wrapped in interruptable here: https://github.com/typelevel/cats-effect/blob/386c27be0596935bccb404e75799c61470c5e8e8/std/shared/src/main/scala/cats/effect/std/ConsoleCrossPlatform.scala#L178

[armanbilge]

Ah, thanks for linking to https://github.com/typelevel/cats-effect/issues/1793, I wasn't aware of that issue. Indeed it seems like IO.readLine is not always cancelable, for reasons outside of our control.

This can be observed in many ways: here, by timeoutTo hanging since it is awaiting cancelation of an uncancelable op.

We do have a timeoutAndForget combinator that wouldn't hang, but it would leak a Thread and probably cause other issues if you try and readLine again later.

https://github.com/typelevel/cats-effect/blob/7887b34b1453131f630ded107540ed11d6fe21ab/core/shared/src/main/scala/cats/effect/IO.scala#L605-L623

[VzxPLnHqr]

Thanks. I can confirm that the following "works" properly but then starts doing weird things on subsequent calls to readLine as you said:

object Main extends IOApp.Simple {
    val run = for {
        _ <- IO.println("enter something in 3 seconds:")
        v <- IO.readLine.timeoutAndForget(3.seconds).handleError(_ => "hello")
        _ <- IO.println(s"you entered $v")
    } yield ()
}

[armanbilge]

Thanks for giving that a try! Yeah, I wouldn't recommend it anyway due to the whole leaking-a-thread thing.

I'm afraid this is a can't-fix on our end. If the JDK did support interruption here it would work as expected, I wonder if its worth filing a bug report with OpenJDK or something.

[VzxPLnHqr]

Yeah, I am not deeply familiar with the jdk itself. It seems like this should be a pretty common use-case though....a console app that expects some input but if it is not supplied after a certain time it moves on with a sensible default. I have not yet looked at how any scala libraries for building console apps handle it.

[VzxPLnHqr]

fs2 seems to have the same problem:

object Main extends IOApp.Simple {
    val run = for {
        v <- fs2.io.stdinUtf8[IO](2).timeout(3.seconds).handleErrorWith(_ => fs2.Stream("hello")).compile.toList
        _ <- IO.println(s"you entered $v")
    } yield ()
}

[armanbilge]

@VzxPLnHqr so here's something, just winging it here without the compiler:

object Main extends IOApp.Simple {
  val mkBetterReadLine: Resource[IO, IO[String]] =
    Resource.eval(Queue.synchronous[IO, String]).flatTap { q =>
      IO.readLine.flatMap(q.offer(_)).foreverM.background
    }.map(q.take)

  val run = mkBetterReadLine.use { betterReadLine =>
      for {
        _ <- IO.println("enter something in 3 seconds:")
        v <- betterReadLine.timeoutAndForget(3.seconds).handleError(_ => "hello")
        _ <- IO.println(s"you entered $v")
    } yield ()
  }
}

The idea is you make a betterReadLine once at the beginning of your program, and use that everywhere else. It will require at most one blocking thread and have non-hanging cancellation without causing problems for subsequent betterReadLine ops.

It may still hang on program termination, which is annoying.

@djspiewak the shittiness of this makes me want to deprecate readLine on JVM too. It's a footgun which can be ameliorated by global state, which really points to a streaming approach like fs2.io.stdin.

[VzxPLnHqr]

@armanbilge thanks. This is an interesting approach and may suffice in the interim for me.

[vasilmkd]

I'm not opposed to deprecating readLine.

[djspiewak]

I think the problem is that everyone wants to use it for their first hello world. It's actually literally in our "Try it now!" snippet. I definitely don't mind clarifying that it's very dangerous in non-trivial apps, but outright deprecating it seems dicy.

[armanbilge]

it's very dangerous in non-trivial apps

I mean ... how many methods in Cats Effect fall into this category? The fact that we're shipping a footgun among a bunch of production-robust methods seems much more dicey to me.

[VzxPLnHqr]

just chiming back in here quickly. @armanbilge I never quite got your little betterReadLine snippet to work (but admittedly, I did not have time to try very hard). However, the more I think about it, it does seem to make sense, from a use-case standpoint, to somehow abstract readLine out like that.

New users (whether to Scala or to cats-effect), need something like readLine to "just work" for them while they are trying to understand the magic that is cats/cats-effect.

Slightly more experienced users, for other reasons, might need readLine to "just work" in the interim while they are putting something together quickly, knowing they might need to refactor it out later.

[VzxPLnHqr]

Also, for some context, what I actually ended up doing was basically making some helper methods like these:

    def prompt(msg: String): IO[String] = IO.print(msg + " ") >> IO.readLine
    def prompt[A](msg: String, default: A)(f: => String => A): IO[A] = promptWithDefault(msg,default)(f)
    def promptBool(msg: String): IO[Boolean] = prompt(msg).map(_.toUpperCase()).map{
      case "Y" | "yes" | "1" => true
      case _ => false
    }
    def promptWithDefault[A](msg: String, default: A)(f: => String => A) = prompt(msg).flatMap{
      case "" => IO(default)
      case s => IO(f(s))
    }

This did not solve the original issue presented here at all, but from a practical standpoint at least allowed me to just hit enter to accept the default value and thereby quickly progress through what might otherwise be a tedious manual testing process.

[armanbilge]

@VzxPLnHqr sorry about that! I took 5 minutes to run it through a compiler, here's a working example that demonstrates timeouts and that betterReadLine still works a second time, even if its cancelled.

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

import cats.effect._
import cats.effect.std._
import cats.syntax.all._
import scala.concurrent.duration._

object Main extends IOApp.Simple {

  override def runtimeConfig =
    // to kill the app when it hangs
    super.runtimeConfig.copy(shutdownHookTimeout = 0.seconds)

  val mkBetterReadLine: Resource[IO, IO[String]] =
    Resource.eval(Queue.synchronous[IO, String]).flatTap { q =>
      IO.readLine.flatMap(q.offer(_)).foreverM.background
    }.map(_.take)

  val run = mkBetterReadLine.use { betterReadLine =>
      for {
        _ <- IO.println("enter something in 3 seconds:")
        v <- betterReadLine.timeoutTo(3.seconds, IO.pure("hello"))
        _ <- IO.println(s"you entered $v")
        _ <- IO.println("enter something else in 3 seconds:")
        v <- betterReadLine.timeoutTo(3.seconds, IO.pure("bye"))
        _ <- IO.println(s"you also entered $v")
    } yield ()
  }
}

[armanbilge]

@VzxPLnHqr thanks again for your feedback ... would you mind qualifying what "just work" means for you? E.g. is the current readLine acceptable and (assuming we can't fix it) do you still think we should keep it as-is or deprecate it? Appreciate your input!

[VzxPLnHqr]

would you mind qualifying what "just work" means for you?

In this context, it means, to me at least, that if readLine is presented to me as an effect which is interruptable/cancellable like any other similar effect, then it should behave as so. But, in this circumstance, it seems there is an issue outside of cats-effect control which is causing the inconsistency.

Playing around with @armanbilge 's workaround solution there (thanks again!), it did at least behave as-expected more, but of course has the downside of now needing to build up this resource and use it, which seems awkward for new people. It "feels like," I should have just been able to change IOApp.Simple to something like IOApp.SimpleWithBetterReadLine and it would "just work" or, if this is a problem enough people run into, the workaround would be incorporated directly into IOApp.Simple.

E.g. is the current readLine acceptable and (assuming we can't fix it) do you still think we should keep it as-is or deprecate it? Appreciate your input!

Once I figured my way around, then it does seem acceptable as-is. So I would not necessarily deprecate it without providing the same functionality elsewhere (like above).

Hope this helps.

[DavidGregory084]

I wonder if it would be sufficient to check in.available() > 0 before calling in.read() in SyncConsole? That should at least tell us whether read() will block.

[armanbilge]

@DavidGregory084 I was thinking along those lines too, except what should we do in the case when available() == 0? If we block, we run into the same problem. Instead we could IO.cede and loop, effectively busy-waiting. But at least cancelable.

[DavidGregory084]

what should we do in the case when available() == 0?

while (in.available() == 0) {}

I'm tempted to put :trollface: but I think it actually would work? It would throw an IOException if System.in was closed and it should be able to be interrupted. On Java 9+ we could put a Thread.onSpinWait inside the loop.

[djspiewak]

Bit of a necro here, but I actually wonder if the busy wait might not be preferable. The other thought I had is that we can probably do a proper readLine if we do some unsafe shenanigans and pull out the file handle to the pipe. I genuinely wonder if that might be worthwhile.

[armanbilge]

I genuinely wonder if that might be worthwhile.

I mean it's just this terrible rabbit hole. Even if we did that (and on Scala Native we can without shenanigans) we'll also have to refactor the read-one-byte-at-a-time loop so that it is capable of async suspension. And the reason that this works at all, so we can have a "stateless" typeclass, is because the standard/system library is doing the dirty work of keeping global state on our behalf. Is it really worth it?

Console#readLine is a lie: it suggests that you can work with stdin imperatively, reading a line whenever you want and changing your mind whenever you want, without maintaining state. You can't. fs2.io.stdin is closer to the right idea: there's a stream of incoming bytes, which you can't really control, only process and/or redirect into some concurrent data structures.

[armanbilge]

I have a proposal to fix the FS2 version of this problem in https://github.com/typelevel/fs2/pull/3093#issuecomment-1366256942.

We can apply a similar fix here, if we are willing to accept a lazy-initialized global single-threaded executor just for reading from stdin. There's already global state at play here, so a global executor doesn't seem completely crazy.

Caveats:

• unless we also maintain some global state, canceling a readLine is effectively turning it into a black hole: the next entered line would be lost, even to subsequent readLines. Actually this is a pretty annoying caveat 🤔

[armanbilge]

Oh also, the in.available() strategy discussed above might not work if the InputStream never returns a value greater than 0. h/t @keynmol for pointing out the default implementation in InputStream just always returns 0. IDK if that's how System.in works, but there may be no guarantee, in which case it would degenerate into an infinite spinloop 😱

[djspiewak]

@armanbilge I've given it some thought, and I think that the solution you ultimately landed in Fs2 seems pretty reasonable here. At least it feels like it replaces one footgun with another which is less severe and less likely to go off randomly.