http: response should always emit close on finish

[trivikr]

Fixes: #17352

Original PR was posted at #17397

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[trivikr]

CI: https://ci.nodejs.org/job/node-test-pull-request/14301/

[trivikr]

Rerun for node-test-commit-freebsd https://ci.nodejs.org/job/node-test-commit-freebsd/17043/

[lpinca]

The !req.res.complete condition is gone, is this wanted?

[apapirovski]

No, definitely not.

[trivikr]

Is there any edit required here?

[apapirovski]

It should have if (!req.res.complete) req.res.emit('aborted');. I would check the rest too. There have been changes to that part of code since that original PR was started.

[trivikr]

Corrected in 8ffc3c7

[apapirovski]

const here.

[trivikr]

changed in 8ffc3c7

[apapirovski]

This will need CITGM run and also, if we don't have something like on-finished in our CitGM then those tests should run separately to make sure.

[BridgeAR]

CITGM https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1365/
CITGM (master) https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1366/

[BridgeAR]

CI https://ci.nodejs.org/job/node-test-pull-request/14309/

[ronag]

I think we need to handle the !res.readable case better, i.e. if response has been emitted on the request object we should always emit aborted on the response object.

  if (res) {
    // *******
    // NOTE: if `req.emit('response')` has been emitted we must always emit `aborted`, regardless of whether the response is readable or not.
    // Socket closed before we emitted 'end' below.
    if (!res.complete) {
      res.emit('aborted');
    }
    // *******
    if (res.readable) {
      res.on('end', function() {
        res.emit('close');
      });
      res.push(null);
    } else {
      res.emit('close');
    }
  }

[ronag]

Should we make the order of req vs res emit close consistent? Right now it is not, i.e we could change it to so that req.emit('close') is always emitted first.

This would avoid intermittent bugs in code that depend on the order.

  if (res) {
    if (res.readable) {
      // Socket closed before we emitted 'end' below.
      if (!res.complete) {
        res.emit('aborted');
      }
      req.emit('close'); // !! Emit close before or after aborted?
      res.on('end', function() {
        res.emit('close');
      });
      res.push(null);
    } else {
      req.emit('close'); // !!
      res.emit('close');
    }
  } else if (!req.socket._hadError) {
    // This socket error fired before we started to
    // receive a response. The error needs to
    // fire on the request.
    req.socket._hadError = true;
    req.emit('error', createHangUpError());
    req.emit('close');  // !!
  } else {
    req.emit('close');  // !!
  }

[ronag]

#20075, feel free to cherry-pick

[BridgeAR]

Ping @trivikr

[trivikr]

Closing this PR as the other one #20075 includes this change (along with additional changes by @ronag)