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

async_hooks,http: fix socket reuse with Agent #13348

Closed
wants to merge 1 commit into from
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/_http_agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ Agent.prototype.addRequest = function addRequest(req, options, port/*legacy*/,
var socket = this.freeSockets[name].shift();
// Assign the handle a new asyncId and run any init() hooks.
socket._handle.asyncReset();
socket[async_id_symbol] = socket._handle.getAsyncId();
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 This is what I was referring to
"Race" was an inexact word

Copy link
Contributor

@refack refack May 31, 2017

Choose a reason for hiding this comment

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

I don't know why but my intuition was to do this somewhere in a Socket method.
Maybe move both lines to a Socket method so other Agents could use it.
Something like Socket.prototype._prepareForReuse() or _recycle

Copy link
Member Author

@addaleax addaleax May 31, 2017

Choose a reason for hiding this comment

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

Hmm … doing it this way doesn’t introduce any new methods that are accessible to the outside world. I am okay with moving it, but that should be based on more consideration than just fixing this bug; as you pointed out in the issue, we would want to consider what kind of API, if any, custom Agent implementations need.

So: I’m not disagreeing, but I think it’s better to postpone that after landing this.

edit: If we do this, we should probably also move the other part (marking the socket as free for reuse).

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed

Copy link
Contributor

Choose a reason for hiding this comment

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

For the moment I'd say this is the correct way to handle the fix, and was an oversight on my part.

@refack

I don't know why but my intuition was to do this somewhere in a Socket method. Maybe move both lines to a Socket method so other Agents could use it. Something like Socket.prototype._prepareForReuse() or _recycle

Technically async_id_symbol was only added as a convenience to skip calling into native and retrieving AsyncWrap::async_id_, but in reality is unnecessary. The most sure way to handle this would be to remove all uses of async_id_symbol where there is a 1:1 pairing of native resource to JS resource and always call getAsyncId().

@addaleax

we would want to consider what kind of API, if any, custom Agent implementations need.

Node allows custom implementation of agents, timers and a few others. Which is the reason why getNewAsyncId() in lib/net.js checks if getAsyncId === 'function' and why insert() in lib/timers.js forcefully adds a new asyncId if none already exists. It has been a total pain during development, and the existing solution is the best I could come up with. Though if you have a better idea I would be happy to try it out.

debug('have free socket');

// don't leak
Expand Down
79 changes: 79 additions & 0 deletions test/parallel/test-async-hooks-http-agent.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const async_id_symbol = process.binding('async_wrap').async_id_symbol;
const http = require('http');

// Regression test for https://github.com/nodejs/node/issues/13325
// Checks that an http.Agent properly asyncReset()s a reused socket handle, and
// re-assigns the fresh async id to the reused `net.Socket` instance.

// Make sure a single socket is transpartently reused for 2 requests.
const agent = new http.Agent({
keepAlive: true,
keepAliveMsecs: Infinity,
maxSockets: 1
});

const server = http.createServer(common.mustCall((req, res) => {
req.once('data', common.mustCallAtLeast(() => {
res.writeHead(200, {'Content-Type': 'text/plain'});
res.write('foo');
}));
req.on('end', common.mustCall(() => {
res.end('bar');
}));
}, 2)).listen(0, common.mustCall(() => {
const port = server.address().port;
const payload = 'hello world';

// First request. This is useless except for adding a socket to the
// agent’s pool for reuse.
const r1 = http.request({
agent, port, method: 'POST'
}, common.mustCall((res) => {
// Remember which socket we used.
const socket = res.socket;
const asyncIdAtFirstRequest = socket[async_id_symbol];
assert.ok(asyncIdAtFirstRequest > 0, `${asyncIdAtFirstRequest} > 0`);
// Check that request and response share their socket.
assert.strictEqual(r1.socket, socket);

res.on('data', common.mustCallAtLeast(() => {}));
res.on('end', common.mustCall(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

in 6bfdeed I used res.socket.once('free'
Might be simpler than on(end, setImmediate)

// setImmediate() to give the agent time to register the freed socket.
setImmediate(common.mustCall(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Probably can leave out common.mustCall() on a setImmediate() but no harm either I suppose.

Copy link
Contributor

Choose a reason for hiding this comment

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

While this isn't an issue, and most likely will never be, there is actually a case that could cause some confusion:

require('async_hooks').createHook({ destroy() { process.exit() }}).enable();
process.on('exit', () => process._rawDebug('exiting...'));
require('net').createServer().listen(0).close();
setImmediate(() => process._rawDebug('setImmediate'));

But that's just for reference. You're most likely correct that's it's unnecessary. But never hurts either. :)

// The socket is free for reuse now.
assert.strictEqual(socket[async_id_symbol], -1);

// Second request. To re-create the exact conditions from the
// referenced issue, we use a POST request without chunked encoding
// (hence the Content-Length header) and call .end() after the
// response header has already been received.
const r2 = http.request({
agent, port, method: 'POST', headers: {
'Content-Length': payload.length
}
}, common.mustCall((res) => {
const asyncId = res.socket[async_id_symbol];
assert.ok(asyncId > 0, `${asyncId} > 0`);
Copy link
Member Author

Choose a reason for hiding this comment

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

Commenting out this assertion will give you back the hard crash that we were seeing in the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Commenting out and removing the fix?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. 😄

assert.strictEqual(r2.socket, socket);
// Empty payload, to hit the “right” code path.
r2.end('');

res.on('data', common.mustCallAtLeast(() => {}));
res.on('end', common.mustCall(() => {
// Clean up to let the event loop stop.
server.close();
agent.destroy();
}));
}));

// Schedule a payload to be written immediately, but do not end the
// request just yet.
r2.write(payload);
}));
}));
}));
r1.end(payload);
}));