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

ClientRequest .abort() and ECONNRESET #32225

Closed
ronag opened this issue Mar 12, 2020 · 5 comments
Closed

ClientRequest .abort() and ECONNRESET #32225

ronag opened this issue Mar 12, 2020 · 5 comments
Labels
http Issues or PRs related to the http subsystem.

Comments

@ronag
Copy link
Member

ronag commented Mar 12, 2020

Continuing the discussion from #32182 (comment)

The current assumption is that always 'error' or 'response' is emitted on the socket.

However, this is not always the case. Consider:

const req = http.get(someUrl);
req.abort();
req.on('error', common.mustCall()); // Fails

This will fail because the case of calling req.abort() before 'socket' will actually result in no error. Whether we get an error or not is a question of timing.

The current semantics are:

  • If abort() before req.'socket', emit req.'close'.
  • If abort() after req.'socket' but before req.'response', emit req.'error'.
  • If abort() after req.'response', emit req.'close' & res.'aborted'.

My main concern (which breaks the above assumption) is the first scenario here. However, I noticed that fixing this will cause lots of other tests to fail.

What can/should we do about this? At a minimum I'd like to clarify the docs.

@ronag ronag changed the title ClientRequest abort() and ECONNRESET ClientRequest .abort() and ECONNRESET Mar 12, 2020
@ronag
Copy link
Member Author

ronag commented Mar 12, 2020

@nodejs/http @mcollina @lpinca

@ronag ronag added the http Issues or PRs related to the http subsystem. label Mar 12, 2020
@mcollina
Copy link
Member

My main concern (which breaks the above assumption) is the first scenario here. However, I noticed that fixing this will cause lots of other tests to fail.

Which test will fail?

@ronag
Copy link
Member Author

ronag commented Mar 13, 2020

Which test will fail?

e.g.

// test/parallel/test-http-abort-before-end.js
'use strict';
const common = require('../common');
const http = require('http');

const server = http.createServer(common.mustNotCall());

server.listen(0, common.mustCall(() => {
  const req = http.request({
    method: 'GET',
    host: '127.0.0.1',
    port: server.address().port
  });

  req.on('abort', common.mustCall(() => {
    server.close();
  }));

  // This should error?
  req.on('error', common.mustNotCall());

  req.abort();
  req.end();
}));

Full list here https://gist.github.com/ronag/379a377e01eb2dfe43c62d54c0220c1a

@jasnell
Copy link
Member

jasnell commented Mar 13, 2020

I actually think the current behavior is fine and it's the assumption that is incorrect. Aborting before the socket is created should not be considered an error condition.

@ronag
Copy link
Member Author

ronag commented Mar 13, 2020

@jasnell: What is your opinion on abort() and destroy() behaving differently in this regards? i.e. with destroy() it is an error condition.

ronag added a commit to nxtedition/node that referenced this issue Apr 13, 2020
Doc deprecates ClientRequest.abort in favor of
ClientRequest.destroy. Also improves event order
documentation for abort and destroy.

Refs: nodejs#32225
ronag added a commit that referenced this issue Apr 26, 2020
Doc deprecates ClientRequest.abort in favor of
ClientRequest.destroy. Also improves event order
documentation for abort and destroy.

Refs: #32225

PR-URL: #32807
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this issue Apr 27, 2020
Doc deprecates ClientRequest.abort in favor of
ClientRequest.destroy. Also improves event order
documentation for abort and destroy.

Refs: #32225

PR-URL: #32807
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR pushed a commit that referenced this issue Apr 28, 2020
Doc deprecates ClientRequest.abort in favor of
ClientRequest.destroy. Also improves event order
documentation for abort and destroy.

Refs: #32225

PR-URL: #32807
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR pushed a commit that referenced this issue Apr 28, 2020
Doc deprecates ClientRequest.abort in favor of
ClientRequest.destroy. Also improves event order
documentation for abort and destroy.

Refs: #32225

PR-URL: #32807
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this issue Apr 28, 2020
Doc deprecates ClientRequest.abort in favor of
ClientRequest.destroy. Also improves event order
documentation for abort and destroy.

Refs: #32225

PR-URL: #32807
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants