No end event is emitted when aborting http/https request

[masnagam]

• Version: v13.7.0
• Platform:
  • Linux (node Docker container running on macOS)
  • macOS Catalina 10.15.3 (Darwin Kernel Version 19.3.0)
• Subsystem: http, https

I'm not sure if this is intended, but I found a difference between v12 and v13 regarding the end event.

// test.js
'use strict';

// The same issue occurs with `http`, too.
const https = require('https');

const req = https.request('https://nodejs.org/dist/v13.7.0/node-v13.7.0.tar.gz');
req.on('response', (res) => {
  res
    .on('end', () => console.log('end'))
    .on('close', () => console.log('close'))
    .on('aborted', () => console.log('aborted'))
    .on('error', (err) => console.log(err));

  setTimeout(() => {
    console.log('abort');
    req.abort();
  }, 500);
});
req.end();

Node.js v12:

$ docker run --rm -it node:12-buster-slim -v
v12.14.1

$ docker run --rm -it -v $(pwd)/test.js:/test.js node:12-buster-slim test.js
abort
aborted
end
close

Node.js v13:

$ docker run --rm -it -v $(pwd)/test.js:/test.js node:13-buster-slim -v
v13.7.0

$ docker run --rm -it -v $(pwd)/test.js:/test.js node:13-buster-slim test.js
abort
aborted
close

This issue might be related to:

• Request streams from http2 intermittantly not emitting end event #31309
• Node.js abort the response if the body length do not match "Content-Length" during 304 #31037

[jasnell]

I believe this change was intentional but pinging @mcollina and @ronag to be certain :-)

[masnagam]

@jasnell Thank you for quick reply.

Is there any way other than abort() to stop a http transaction that always emits an end event before closed?

At least, replacing req.abort() with res.destroy() cannot solve:

    setTimeout(() => {
      console.log('abort');
-     req.abort();
+     res.destroy();
    }, 500);

Changing like below may solve:

      .on('aborted', () => console.log('aborted'))
+     .on('aborted', () => res.readableEnded || res.emit('end'))
      .on('error', (err) => console.log(err));

but causes an issue in v12:

$ docker run --rm -it -v $(pwd)/test.js:/test.js node:12-buster-slim test.js
abort
aborted
end
end
close

[mcollina]

The change is deliberate, because 'end' signals a successful completion of a stream, and an aborted stream is not successful.

@masnagam from your comment it is not clear what problem this change causes.

[masnagam]

@mcollina Thank you for you comment.

That's ok to me if it's intentional.

In Node.js v12, the end event is always emitted. So, I used it for detecting the end of the response stream. But, maybe, it's not correct way. I guess that we should use the close event for that purpose.

[mcollina]

If you are interested in knowing about both successful and aborted requests, you should use 'close'.

[masnagam]

Now, I'm investigating a case of res.pipe(stream).

If stream expects the end event, res should emit it even when it aborted.

[mcollina]

Can you please post a full example to reproduce your problem?

[masnagam]

This change might cause problem when piping.

I modified my test program like below:

// test.js

const https = require('https');
const { Transform } = require('stream');

class MyTransform extends Transform {
  constructor(options) {
    super(options);
  }

  _transform(chunk, encoding, callback) {
    callback();
  }

  _flush(callback) {
    console.log('flush');
    callback();
  }
}

const req = https.request('https://nodejs.org/dist/v13.7.0/node-v13.7.0.tar.gz');
req.on('response', (res) => {
  res
    .on('end', () => console.log('end'))
    .on('close', () => console.log('close'))
    .on('aborted', () => console.log('aborted'))
    .on('error', (err) => console.log(err));

  res.pipe(new MyTransform);

  setTimeout(() => {
    console.log('abort');
    req.abort();
  }, 500);
});
req.end();

In Node.js v13, _flush() is never called.

$ docker run --rm -it -v $(pwd)/test.js:/test.js node:13-buster-slim test.js
abort
aborted
close

$ docker run --rm -it -v $(pwd)/test.js:/test.js node:12-buster-slim test.js
abort
aborted
end
flush
close

After changing like below:

      .on('aborted', () => console.log('aborted'))
+     .on('aborted', () => res.readableEnded || res.emit('end'))
      .on('error', (err) => console.log(err));

$ docker run --rm -it -v $(pwd)/test.js:/test.js node:13-buster-slim test.js
abort
aborted
end
flush
close

[mcollina]

That is normal. Note that there are a few other cases where your stream would not terminate properly (as an example, in case of network errors). The "proper" behavior for streams in that case is that it should not call _flush.

Use this:

const https = require('https');
const { Transform, pipeline } = require('stream');

class MyTransform extends Transform {
  constructor(options) {
    super(options);
  }

  _transform(chunk, encoding, callback) {
    callback();
  }

  _flush(callback) {
    console.log('flush');
    callback();
  }
}

const req = https.request('https://nodejs.org/dist/v13.7.0/node-v13.7.0.tar.gz');
req.on('response', (res) => {
  res
    .on('end', () => console.log('end'))
    .on('close', () => console.log('close'))
    .on('aborted', () => console.log('aborted'))
    .on('error', (err) => console.log(err));

  pipeline(res, new MyTransform, function (err) {
    if (err) { console.error(err) } // this will have an error, because it's an error situation.
  });

  setTimeout(() => {
    console.log('abort');
    req.abort();
  }, 500);
});
req.end();

[masnagam]

Thank you for your explanation!

I fully understood my mistake. Thank you.