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

http: ignore socket errors after req.abort #20077

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions doc/api/http.md
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,8 @@ added: v0.3.8
-->

Marks the request as aborting. Calling this will cause remaining data
in the response to be dropped and the socket to be destroyed.
in the response to be dropped and the socket to be destroyed. After
calling this method no further errors will be emitted.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking nit-pick:

Suggested change
calling this method no further errors will be emitted.
calling this method, no further errors will be emitted.


### request.aborted
<!-- YAML
Expand Down Expand Up @@ -2138,8 +2139,6 @@ will be emitted in the following order:
* `'socket'`
* (`req.abort()` called here)
* `'abort'`
* `'error'` with an error with message `'Error: socket hang up'` and code
`'ECONNRESET'`
* `'close'`

If `req.abort()` is called after the response is received, the following events
Expand Down
16 changes: 12 additions & 4 deletions lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,9 @@ function socketCloseListener() {
// receive a response. The error needs to
// fire on the request.
req.socket._hadError = true;
req.emit('error', connResetException('socket hang up'));
if (!req.aborted) {
req.emit('error', connResetException('socket hang up'));
}
}
req.emit('close');
}
Expand All @@ -399,7 +401,9 @@ function socketErrorListener(err) {
// For Safety. Some additional errors might fire later on
// and we need to make sure we don't double-fire the error event.
req.socket._hadError = true;
req.emit('error', err);
if (!req.aborted) {
req.emit('error', err);
}
}

// Handle any pending data
Expand Down Expand Up @@ -433,7 +437,9 @@ function socketOnEnd() {
// If we don't have a response then we know that the socket
// ended prematurely and we need to emit an error on the request.
req.socket._hadError = true;
req.emit('error', connResetException('socket hang up'));
if (!req.aborted) {
req.emit('error', connResetException('socket hang up'));
}
}
if (parser) {
parser.finish();
Expand All @@ -455,7 +461,9 @@ function socketOnData(d) {
freeParser(parser, req, socket);
socket.destroy();
req.socket._hadError = true;
req.emit('error', ret);
if (!req.aborted) {
req.emit('error', ret);
}
} else if (parser.incoming && parser.incoming.upgrade) {
// Upgrade (if status code 101) or CONNECT
var bytesParsed = ret;
Expand Down
23 changes: 23 additions & 0 deletions test/parallel/test-http-client-aborted.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
'use strict';

const common = require('../common');
const http = require('http');
const assert = require('assert');

const server = http.createServer(common.mustCall(function(req, res) {
req.on('aborted', common.mustCall(function() {
assert.strictEqual(this.aborted, true);
server.close();
}));
assert.strictEqual(req.aborted, false);
res.write('hello');
}));

server.listen(0, common.mustCall(() => {
const req = http.get({
port: server.address().port,
headers: { connection: 'keep-alive' }
}, common.mustCall((res) => {
req.abort();
}));
}));
21 changes: 21 additions & 0 deletions test/parallel/test-http-client-no-error-after-aborted.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
'use strict';

const common = require('../common');
const http = require('http');

const server = http.createServer(common.mustCall((req, res) => {
res.write('hello');
}));

server.listen(0, common.mustCall(() => {
const req = http.get({
port: server.address().port
}, common.mustCall((res) => {
req.on('error', common.mustNotCall());
req.abort();
req.socket.destroy(new Error());
req.on('close', common.mustCall(() => {
server.close();
}));
}));
}));
3 changes: 1 addition & 2 deletions test/parallel/test-http-client-timeout-on-connect.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@ server.listen(0, common.localhostIPv4, common.mustCall(() => {
}));
}));
req.on('timeout', common.mustCall(() => req.abort()));
req.on('error', common.mustCall((err) => {
assert.strictEqual(err.message, 'socket hang up');
req.on('abort', common.mustCall(() => {
server.close();
}));
}));
2 changes: 1 addition & 1 deletion test/parallel/test-http-writable-true-after-close.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ const server = createServer(common.mustCall((req, res) => {
}));
}).listen(0, () => {
external = get(`http://127.0.0.1:${server.address().port}`);
external.on('error', common.mustCall(() => {
external.on('abort', common.mustCall(() => {
server.close();
internal.close();
}));
Expand Down