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

stream.finished failing to detect a destroyed ServerResponse #34301

Closed
tiagonapoli opened this issue Jul 10, 2020 · 2 comments
Closed

stream.finished failing to detect a destroyed ServerResponse #34301

tiagonapoli opened this issue Jul 10, 2020 · 2 comments
Labels
stream Issues and PRs related to the stream subsystem.

Comments

@tiagonapoli
Copy link

tiagonapoli commented Jul 10, 2020

  • Version: v14.5.0
  • Platform: Linux - kernel 5.4.0-40-generic
  • Subsystem: Stream utils (?)

What steps will reproduce the bug?

stream.finished is failing to detect a closed ServerResponse in the case the request is aborted (response emits close but doesn't emit finish).

This is happening when stream.finished is called with an already closed ServerResponse. I thought it should work in this case because if I give a destroyed PassThrough stream for example it works properly - it executes the callback.

const stream = require("stream");

const pass = new stream.PassThrough();
pass.on("close", () => {
  stream.finished(pass, (err) => {
    // it works
    console.log("stream.finished worked", err);
  });
});

pass.destroy();

Now in the case of a response the callback isn't executed:

const stream = require("stream");
const http = require("http");

http
  .createServer((req, res) => {
    res.on("finish", () => {
      // res will not be finished
      console.log("response finished")
    });
    res.on("close", () => {
      // res will be closed
      console.log("response closed");
      stream.finished(res, (err) => {
        // but this callback will not be executed
        console.log("stream.finished worked");
      });
    });

    res.statusCode = 200;
    res.setHeader("Content-Type", "text/plain");
    res.write("Hello world");
  })
  .listen(3000, "localhost", () => {
    console.log(`Server running at http://localhost:3000/`);
    const req = http.get("http://localhost:3000", (res) => {
      console.log("Request was made");
      setTimeout(() => req.abort(), 100);
    });
  });

What is the expected behavior?

I expected console output to be:

Server running at http://localhost:3000/
Request was made
response closed
stream.finished worked

What do you see instead?

The output is:

Server running at http://localhost:3000/
Request was made
response closed
@lpinca lpinca added the stream Issues and PRs related to the stream subsystem. label Jul 11, 2020
@lpinca
Copy link
Member

lpinca commented Jul 11, 2020

@nodejs/streams @ronag

@ronag
Copy link
Member

ronag commented Jul 11, 2020

Server response is not a real stream and doesn’t have the exact same semantics. finished on destroyed stream only works on “modern” streams since v14.

I’ll see if we can add special handling for server response.

ronag added a commit to nxtedition/node that referenced this issue Jul 12, 2020
finished should invoke callback on closed OutgoingMessage the
same way as for regular streams.

Fixes: nodejs#34301
@ronag ronag closed this as completed in a55b77d Jul 16, 2020
cjihrig pushed a commit that referenced this issue Jul 23, 2020
finished should invoke callback on closed OutgoingMessage the
same way as for regular streams.

Fixes: #34301

PR-URL: #34313
Fixes: #34274
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants