Skip to content

Commit

Permalink
http: fix test where aborted should not be emitted
Browse files Browse the repository at this point in the history
PR-URL: #20077
Fixes: #20107
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
  • Loading branch information
ronag authored and Trott committed Jul 15, 2019
1 parent 2550ddb commit 461bf36
Show file tree
Hide file tree
Showing 6 changed files with 60 additions and 10 deletions.
5 changes: 2 additions & 3 deletions doc/api/http.md
Original file line number Diff line number Diff line change
Expand Up @@ -541,7 +541,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.

### request.aborted
<!-- YAML
Expand Down Expand Up @@ -2142,8 +2143,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 @@ -374,7 +374,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 @@ -400,7 +402,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 @@ -434,7 +438,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 @@ -457,7 +463,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

0 comments on commit 461bf36

Please sign in to comment.