-
Notifications
You must be signed in to change notification settings - Fork 602
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix writeUtf8Lines
evaluates leading effects in the input stream twice
#3489
base: main
Are you sure you want to change the base?
Conversation
@@ -464,8 +464,13 @@ sealed trait Files[F[_]] extends FilesPlatform[F] { | |||
def writeUtf8Lines(path: Path, flags: Flags): Pipe[F, String, Nothing] = in => | |||
in.pull.uncons | |||
.flatMap { | |||
case Some(_) => | |||
in.intersperse(lineSeparator).append(Stream[F, String](lineSeparator)).underlying | |||
case Some((next, rest)) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just remove the in.pull.uncons.flatMap
entirely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we keep the “don't add lineSeparator if the stream is empty” specification, I don't think we can remove in.pull.uncons.flatMap
since we cannot determine if the stream is empty until we evaluate the first element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, ok... can we test a bit with scopes? I worry, perhaps unnecessarily, that the proposed fix might mess with resource finalization -- i.e. some resources might get finalized early.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like any resource that was to be released at the end of the input stream also gets released at the end of the uncons
ed-and-flatMap
ed stream.
I was chatting with @kory33 about this potential corner case. His opinion was that uncons
is not specified enough, and that we may want to have additional test-cases on uncons
to ensure such early finalizations do not occur.
Code
val program = Stream
.bracketWeak(IO { println("Resource 1 acquired"); "1" })(_ => IO.println("Resource 1 released"))
.append(
Stream
.bracketWeak(IO { println("Resource 2 acquired"); "2" })(_ => IO.println("Resource 2 released"))
.append(Stream.emit("3"))
.append(Stream.exec(IO.println(s"test 1")))
.scope
)
.append(Stream.exec(IO.println(s"test 2")))
.through(Files.forIO.writeUtf8Lines(Path("test.txt")))
.compile
.drain
program.unsafeRunSync()
Output
Resource 1 acquired
Resource 2 acquired
test 1
Resource 2 released
test 2
Resource 1 released
Fixes #3488.
There was a problem with
writeUtf8Lines
executingin.pull.uncons
and evaluating leading effects, but discarding the result and usingin
.(Sorry if it is difficult to read due to the use of translation software.)