Make Console#readLine cancelable
#3465
Conversation
| @deprecated("Use overload with Async constraint", "3.5.0") | ||
| def make[F[_]](F: Sync[F]): Console[F] = F match { |
There was a problem hiding this comment.
I know, I know, v3.5.0 shop is closed. But ... we could fix this beginner stumbling block once and for all 😇
No strong feelings 😁
There was a problem hiding this comment.
Oh actually, we can fix this source-compatibly and get it into 3.5.x. We can't expose the new instance publicly without a source-breaking change, but we can replace the instance for IO right now, and do the source-breaking change in 3.6. This gets us most of the win.
| } else { // happy days! | ||
| callback(line) |
There was a problem hiding this comment.
Something that is nagging at me is that this is a leak here. The callback may have already be canceled, but we are invoking it anyway. There seems no reliable way to know for sure.
I don't see any way to plug this without changing the callback in async { cb => to return a Boolean indicating whether it completed successfully or not. I feel like we've discussed this before.
| /** | ||
| * Constructs a `Console` instance for `F` data types that are [[cats.effect.kernel.Async]]. | ||
| */ | ||
| def make[F[_]](implicit F: Async[F]): Console[F] = |
There was a problem hiding this comment.
Isn't this now the same constraint on both JS and JVM? Do we need the platform split?
There was a problem hiding this comment.
Well, at the moment there's still Native with the Sync constraint. I can revisit that when we get polling system and/or Scala Native multithreading.
Fixes #3077. This change is source-breaking, because it upgrades the constraint on
ConsolefromSynctoAsync.The fix is inspired by typelevel/fs2#3093 (comment). We now run all
readLineWithCharsetoperations on a dedicated daemon thread, that is lazily started.This
stdinReaderruns a simple state machine. It waits for aReadLineRequest, then it reads a line from stdin, then it completes the request callback. If the request was canceled in the meantime, then it saves this line for the next request, which might already be pending.This implementation never drops a line. If it has a line cached from a previous call that was canceled, the next request will either be completed with said line, or it will be failed with an
IllegalStateExceptionif the charset does not match the charset the cached line was read with.I manually verified that this fixes the example given in #3077, as well as a couple other examples that I played with. I also verified that the
cats-effect-stdinthread only appears if you specifically touchreadLine.