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

http2: respondWithFile/respondWithFD events and stream API interaction #20014

Closed
3 tasks
jasnell opened this issue Apr 13, 2018 · 4 comments
Closed
3 tasks

http2: respondWithFile/respondWithFD events and stream API interaction #20014

jasnell opened this issue Apr 13, 2018 · 4 comments

Comments

@jasnell
Copy link
Member

jasnell commented Apr 13, 2018

/cc @mcollina @addaleax @mafintosh @nodejs/http2

Currently in ServerHttp2Stream, when respondWithFile() or respondWithFD() are called, the Writable side of the Duplex is closed, causing the finish event to be emitted even tho data is still actually flowing (albeit at the C++ layer). Other than the close event, there is currently no way for user code to know when the Http2Stream is done sending the data internally, and by the time we get the close event, there's nothing else that can be done with the Http2Stream because it's... well... closed.

When using the Duplex writable side, when finish is called the Http2Stream is not yet closed so there are still things that can be done with it.

A second issue is that user code should not call write() or end() on the Http2Stream after calling either of the respondWithFile() or respondWithFD() methods. Both should raise an error because the user is explicitly opting out of using the Writable API at that point.

So there are a couple of todo's here:

  • Throw if write() or end() is called on Http2Stream by user code after respondWithFile() or respondWithFD() are used.

  • Emit finish on the Http2Stream after the C++ side is done writing the data from the FD but before the stream is closed.

  • As an alternative to emitting finish, the respondWithFile() and respondWithFD() calls could take a callback that is invoked when the data send is complete and before the stream is closed. (Personally this one would be my preference)

@addaleax
Copy link
Member

Just for context, I am still going to work on giving .pipe() a fast path for C++ streams, which I would like to eventually use to make h2stream.respondWithFile() equivalent to fs.createReadStream().pipe(h2stream).
(Some WIP is @ addaleax/node@98f7572, but right now it only supports network sockets & http2 streams so it’s not all too useful.)

Since that would be a fast path for .pipe(), retaining all or almost all existing behaviour, the finish event would have to be kept intact. As a consequence, I would prefer the the second option of emitting finish over a callback to the respondWith* functions.

@mafintosh
Copy link
Member

Sounds like you should impl _final and have it wait for everything to close down nicely before calling it's cb. That'll make finish emit only after the stream is fully done but any subsequent calls to write will throw as they should

@jasnell
Copy link
Member Author

jasnell commented Apr 14, 2018

Oh nice, that may be exactly what we need. I'll investigate that a bit.

@mafintosh
Copy link
Member

Added benefit is that you become a good stream citizen and all the tooling will work as expected with http2 streams

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

No branches or pull requests

3 participants