Fix bug in Pipe.fold where sink can incorrectly be left open #1413
Conversation
| if (closed) { | ||
| sink.close() | ||
| } else { | ||
| sink.flush() |
There was a problem hiding this comment.
It wasn't clear if this flush vs. close was really necessary. It felt like an optimization. I changed it because I didn't want the code to inadvertently close the sink 2x, and also didn't want to add yet-another piece of state to prevent such a double-close. The code seemed a little easier on the eyes to simply have the close called in one place, and accept the potential overhead of this explicit flush right before close.
There was a problem hiding this comment.
inadvertently close the sink 2x
The documentation of Sink requires the second call to be a no-op. So if it simplifies things here, you are free to do it.
Also a call to close() implies flush(), so you don't need to explicitly flush() if you know you're going to close (I didn't review the actual change).
There was a problem hiding this comment.
I don't know that it simplifies anything to let it call close here, too. It would be an extra conditional and a couple of extra lines that IMO aren't much value. Feels like we should only be bothering to call close at the end.
I was just explaining that by moving the call to close to the end, we could end up calling flush needlessly, because we'll call it in the last iteration of the loop for the last chunk of data, but we might then immediately call close on return. We don't know for certain here whether we're going to close, so we do need the flush. A possible alternative I thought of would be to defer calling flush until we return from the loop. But I don't think that's the way to go because it could induce unsynchronized concurrent access to the destination sink -- by calling flush in the folding thread, which is stated to be safe to call concurrently with calls to write to the pipe's sink, which could also write to the destination sink, but from a different thread.
Sorry if that paragraph is hard to parse. Maybe it would be more clear to just review the change and see if it looks fine or if you'd prefer an alternate formulation. (It's small; most of the lines of code in this PR are the new test case.)
There was a problem hiding this comment.
I'm on mobile which is why I didn't. I'll reread this and review the diff later or tomorrow on a nice big screen.
There was a problem hiding this comment.
(gentle reminder, if you have a moment to look)
There was a problem hiding this comment.
I did take a look, and it only served to convince me that @swankjesse should be the one looking!
This adds a new test case that demonstrates the bug described in #1412.
This also proposes a fix, by making sure the final loop iteration -- when the buffer is empty and the new destination is swapped in -- can close the destination sink if necessary.
Resolves #1412