Skip to content
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

Should pipeTo return a promise for when the piping completes? #236

Closed
domenic opened this issue Oct 21, 2014 · 10 comments
Closed

Should pipeTo return a promise for when the piping completes? #236

domenic opened this issue Oct 21, 2014 · 10 comments

Comments

@domenic
Copy link
Member

domenic commented Oct 21, 2014

In an offline thread, @acolwell was looking at a scenario where you'd do rs.pipeTo(ws, { preventClose: true }) and wondering how to know when the piping was finished. This would be important for e.g. concatenating multiple readable streams into a writable stream.

You could do this by monitoring rs.closed, but that feels a bit oblique and inverted. We could have pipeTo return a promise that is fulfilled when the piping completes (or rejected if it errors).

If we do this, would "piping completes" best correspond to (a) all data successfully read from rs; or (b) all data successfully written to ws? I would lean toward (b).

@tyoshino
Copy link
Member

Currently, we have only pipeTo() that finishes when rs is "closed" or "errored" (i). IIRC, there were some discussion about partial pipeTo() where we can specify the amount to pipe (e.g. number of elements) (ii).


Thinking of only (i), we can get notified of completion of reading from rs by watching rs.closed as you said though it's oblique. What's really missing is the way to know completion of write to ws. That is (b).

(a) is useful for type-(ii) pipeTo() since it allows the user to start new pipeTo() from rs asap when data of the specified amount is read (only successful case matters. when rs errors, ws is aborted so we can know that by (b) type promise). But it's useless for type-(i). (b) is useful to avoid conflict of pipeTo operations to the same destination for both type-(i) and (ii) pipeTo.

I guess we give a different name to type-(ii) pipeTo even if we introduce it, so, I think it's ok to adopt the semantics (b) for pipeTo().


Before in similar discussion I proposed making pipeTo return a tuple (or dictionary) of pipeToPromise and dest. But Domenic pointed out that it prevents chain style coding (a.pipeTo(b).pipeTo(c)). I'm just thinking if we can add pipeTo method to the object to return so that we can write the pipeTo chain. Do you think it works?

@domenic
Copy link
Member Author

domenic commented Oct 22, 2014

Before in similar discussion I proposed making pipeTo return a tuple (or dictionary) of pipeToPromise and dest. But Domenic pointed out that it prevents chain style coding (a.pipeTo(b).pipeTo(c)). I'm just thinking if we can add pipeTo method to the object to return so that we can write the pipeTo chain. Do you think it works?

I remember this now. The objection was slightly different though, since the chain is usually a.pipeThrough(b).pipeTo(c). The only thing you want to do with the end of the chain is usually a.pipeThrough(b).pipeTo(c).closed.then(...), I think. In that case changing it to return a piping-finished promise seems OK since they're equivalent in the { preventClose: false } case.

@tyoshino
Copy link
Member

I see. Ah, we need to discuss also pipeThrough(). Regarding pipeThrough(), it seems it's useless to know when writing to b's readable is complete (semantics (b)). Right? We might want to know when b is ready for reuse for another work, but for that purpose, we need to know completion of .pipeTo(c)? Hmm, in the first place, transform streams are single use (reusable but only after some reset operation?)?

In that case changing it to return a piping-finished promise seems OK since they're equivalent in the { preventClose: false } case.

Wow!

@domenic
Copy link
Member Author

domenic commented Oct 22, 2014

I think the whole point of pipeThrough is to give a nice chainable sugar, so it should remain as-is. If you want to know more details you can manually do the pipeTo.

@domenic
Copy link
Member Author

domenic commented Oct 22, 2014

Hmm, in the first place, transform streams are single use (reusable but only after some reset operation?)?

Hmm, this is interesting. I didn't really think about this very much. I guess they could be reused if you didn't close either end? And you'd need to wait for them to finish flushing everything through? Kind of strange.

@tyoshino
Copy link
Member

For example, a DEFLATE-ing transform stream basically needs to be reset to use for another work since it has a compression context (LZ77 window remembering history for back-reference). So, I'd ask user to close writable side and fully read from readable side (I'd design it to become "closed" once all data is read) and ask the user to call some resetting method. For this use case, we can just watch readable's closed to see when we can do resetting.

I guess it's rare that we want to have data go through a transform stream and use it for another work without reset (where something (say, pipeThroughPromise) other than readable's closed would be useful). E.g. compressing big file using the same compression context but writing into separate files? Hmm... Maybe we don't need to cover these use case by Streams API.

@domenic
Copy link
Member Author

domenic commented Oct 22, 2014

For example, a DEFLATE-ing transform stream basically needs to be reset to use for another work since it has a compression context

For that example I'd just create a new instance of the DEFLATE-ing transform stream :).

E.g. compressing big file using the same compression context but writing into separate files?

I think this could be accomplished, but you might have to drop down to trickier, primitives. E.g. either rewrite pipeTo yourself, or turn off all auto-closing/aborting/canceling, or use some sort of tee stream. No need to support it out of the box I think.

@tyoshino
Copy link
Member

So, in summary...

  • even if we would have a reusable transform stream, we'll write code to watch it's readable side's closed and call next pipeThrough()/pipeTo() manually to reset and use the transform stream for another work asap if we want. And, in most cases, we just chain pipeThrough() to the next pipeThrough()/pipeTo().
  • change pipeTo() to return a promise that will be fulfilled when all data is written to dest (or when dest is successfully closed if preventClose is false), and be rejected when dest is errored (maybe it's ok to fast-fail when rs is errored)?

@tyoshino
Copy link
Member

Oh, it's not fast-fail since dest.abort() changes the state of dest to "errored" sync.

Hmm, ... no, we have preventAbort option. Hmm, we should just prototype in reference impl.

@tyoshino
Copy link
Member

... I'd just create a new instance ...

Yep :)

... No need to support it out of the box ...

OK. Got it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants