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.

Sign up for GitHub

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

Jump to bottom

http: align destroy w streams #32182

Closed
wants to merge 3 commits into from
Closed

http: align destroy w streams #32182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fa3b428
http: align destroy w streams
ronag Mar 10, 2020
6ce7477
fixup: test
ronag Mar 10, 2020
7ef95e4
fixup: test
ronag Mar 10, 2020
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: 1 addition & 4 deletions lib/_http_client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,9 +375,6 @@ function _destroy(req, socket, err) {
socket.destroy(err);
} else {
socket.emit('free');
if (!req.aborted && !err) {
err = connResetException('socket hang up');
Copy link
Member

Choose a reason for hiding this comment

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

I don't like to remove this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Any specific reason? If I call e.g. .destroy() on a fs stream it won't cause any error (or any other stream I can think of at the moment).

Is it backwards compat which is you concern?

Copy link
Member

Choose a reason for hiding this comment

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

It's an informative error that is being removed for no good reason. What's the problem with it? It doesn't work 100% like stream.destroy() but it is really so problematic to justify a semver-major change?

Copy link
Member Author

@ronag ronag Mar 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I would argue that destroying/aborting a stream is not an error and emitting an error is weird/confusing since for me that would indicate something went wrong / unexpected happened.

I do agree that the worth of the change is questionable though.

}
if (err) {
req.emit('error', err);
}
Expand Down Expand Up @@ -427,7 +424,7 @@ function socketCloseListener() {
res.emit('close');
}
} else {
if (!req.socket._hadError) {
if (!req.destroyed && !req.socket._hadError) {
// This socket error fired before we started to
// receive a response. The error needs to
// fire on the request.
Expand Down
3 changes: 1 addition & 2 deletions test/parallel/test-http-client-abort-destroy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,7 @@ const assert = require('assert');
server.listen(0, common.mustCall(() => {
const options = { port: server.address().port };
const req = http.get(options, common.mustNotCall());
req.on('error', common.mustCall((err) => {
assert.strictEqual(err.code, 'ECONNRESET');
req.on('close', common.mustCall((err) => {
Copy link
Member

Choose a reason for hiding this comment

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

I think this change is not correct.

A user is currently adding req.on('error') and req.on('response'). They are not adding req.on('close').

This comment was marked as off-topic.

Sign in to view

Copy link
Member Author

@ronag ronag Mar 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This confuses me a little though, abort() is what most people use (I think?) and then according to your comment above they are already broken.

I think we have two options here:

  • Make destroy() not force an error. (This PR currently)
  • Make abort() force an error.

Having the two of them behave differently is confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to clarify, this is not about swallowing errors. This is about forcing an error.

Copy link
Member

Choose a reason for hiding this comment

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

The current behavior of http.request() implies that either 'error' is fired or 'response' is fired. Firing nothing wil definitely create a memory leak.

Copy link
Member Author

@ronag ronag Mar 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Then abort() has been broken for a long time, because you can call abort() and get neither 'error' nor 'response'.

This happens if abort() is called before the request has received a socket (previously through the short circuit in onSocket).

Should I change this PR to fix that instead?

Firing nothing wil definitely create a memory leak.

'close' is always fired, but I think I get your point.

Copy link
Member

Choose a reason for hiding this comment

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

I'm talking about code that has already been written and will break in subtle ways.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll open a separate PR to fix abort

server.close();
}));
assert.strictEqual(req.aborted, false);
Expand Down
8 changes: 1 addition & 7 deletions test/parallel/test-http-client-abort-keep-alive-queued-tcp-socket.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,7 @@ for (const destroyer of ['destroy', 'abort']) {
const req = http.get({ agent, port }, common.mustNotCall());
req[destroyer]();

if (destroyer === 'destroy') {
req.on('error', common.mustCall((err) => {
assert.strictEqual(err.code, 'ECONNRESET');
}));
} else {
req.on('error', common.mustNotCall());
}
req.on('error', common.mustNotCall());

http.get({ agent, port }, common.mustCall((res) => {
res.resume();
Expand Down
11 changes: 1 addition & 10 deletions test/parallel/test-http-client-close-event.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,24 +5,15 @@ const common = require('../common');
// event when a request is made and the socket is closed before we started to
// receive a response.

const assert = require('assert');
const http = require('http');

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

server.listen(0, common.mustCall(() => {
const req = http.get({ port: server.address().port }, common.mustNotCall());
let errorEmitted = false;

req.on('error', common.mustCall((err) => {
errorEmitted = true;
assert.strictEqual(err.constructor, Error);
assert.strictEqual(err.message, 'socket hang up');
assert.strictEqual(err.code, 'ECONNRESET');
}));

req.on('error', common.mustNotCall());
req.on('close', common.mustCall(() => {
assert.strictEqual(errorEmitted, true);
server.close();
}));

Expand Down
8 changes: 2 additions & 6 deletions test/parallel/test-http-client-set-timeout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
// Test that the `'timeout'` event is emitted exactly once if the `timeout`
// option and `request.setTimeout()` are used together.

const { expectsError, mustCall } = require('../common');
const { mustNotCall, mustCall } = require('../common');
const { strictEqual } = require('assert');
const { createServer, get } = require('http');

Expand Down Expand Up @@ -31,11 +31,7 @@ server.listen(0, mustCall(() => {
}));
}));

req.on('error', expectsError({
name: 'Error',
code: 'ECONNRESET',
message: 'socket hang up'
}));
req.on('error', mustNotCall());

req.on('close', mustCall(() => {
server.close();
Expand Down
3 changes: 1 addition & 2 deletions test/parallel/test-http-client-timeout-on-connect.js
View file
Open in desktop
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('close', common.mustCall((err) => {
server.close();
}));
}));
2 changes: 1 addition & 1 deletion test/parallel/test-http-writable-true-after-close.js
View file
Open in desktop
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('close', common.mustCall(() => {
server.close();
internal.close();
}));
Expand Down
6 changes: 1 addition & 5 deletions test/parallel/test-https-timeout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,7 @@ const server = https.createServer(options, function() {
console.log('got response');
});

req.on('error', common.expectsError({
message: 'socket hang up',
code: 'ECONNRESET',
name: 'Error'
}));
req.on('error', common.mustNotCall());

req.on('timeout', common.mustCall(function() {
console.log('timeout occurred outside');
Expand Down