search

typelevel / fs2

Compositional, streaming I/O library for Scala
https://fs2.io
Other
2.36k stars 596 forks source link

`fs2.io.readOutputStream` doesn't preserve errors from `cats.data.EitherT` #3246

Open mrdziuban opened 1 year ago

mrdziuban commented 1 year ago

Using the latest fs2 version 3.7.0 and an effect type that includes cats.data.EitherT, an error in the effect is not preserved when using fs2.io.readOutputStream. Possibly related to #2821

Minimized reproduction:

https://scastie.scala-lang.org/mrdziuban/qLvScG5BSWiLsXlhB5pJYg/2

import cats.data.EitherT
import cats.effect.IO
import cats.effect.unsafe.implicits.global

type Eff[A] = EitherT[IO, String, A]

fs2.io.readOutputStream[Eff](1024)(_ => EitherT.left(IO.pure("error")))
  .compile
  .drain
  .value
  .unsafeRunSync()
// Right(())
armanbilge commented 1 year ago

I've given an explanation in https://github.com/typelevel/fs2/issues/3199#issuecomment-1496592183 why EitherT in general doesn't work well with streams. I know it doesn't help immediately, but the long-term solution is a strategy like proposed in https://github.com/typelevel/cats-mtl/pull/489.

Not to say this very specific issue isn't fixable, perhaps it is. But monad transformer behavior in general is not fixable.

mrdziuban commented 1 year ago

Thanks for the links @armanbilge! I've refactored my code to remove EitherT so at least this isn't blocking me.

This heads towards architectural questions, but given

monad transformer behavior in general is not fixable

should the default behavior of fs2 be to not allow stream usage with monad transformers (or for cats to not provide the fallback Concurrent instance)? I personally would have preferred a compile error to the unexpected runtime behavior

armanbilge commented 1 year ago

or for cats to not provide the fallback Concurrent instance

This is a very good point. I feel inclined to agree, these instances seem more harmful than good, and actually the trouble starts from MonadCancel although I'm not sure if it is observable until Concurrent. This is a discussion for the Cats Effect repository, I can open that ...