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: set async_id_symbol to any socket set #14389

Closed
wants to merge 1 commit into from
Closed

http: set async_id_symbol to any socket set #14389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 16 additions & 1 deletion lib/_http_outgoing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,10 @@ const checkIsHttpToken = common._checkIsHttpToken;
const checkInvalidHeaderChar = common._checkInvalidHeaderChar;
const outHeadersKey = require('internal/http').outHeadersKey;
const async_id_symbol = process.binding('async_wrap').async_id_symbol;
const async_hooks = require('async_hooks');
const nextTick = require('internal/process/next_tick').nextTick;
const errors = require('internal/errors');
const kSocket = Symbol('socket');

const CRLF = common.CRLF;
const debug = common.debug;
Expand Down Expand Up @@ -116,7 +118,7 @@ function OutgoingMessage() {
this.finished = false;
this._headerSent = false;

this.socket = null;
this[kSocket] = null;
this.connection = null;
this._header = null;
this[outHeadersKey] = null;
Expand All @@ -125,6 +127,19 @@ function OutgoingMessage() {
}
util.inherits(OutgoingMessage, Stream);

Object.defineProperty(OutgoingMessage.prototype, 'socket', {
get: function() {
return this[kSocket];
},
set: function(socket) {
this[kSocket] = socket;
if (socket && socket[async_id_symbol] === undefined) {
socket[async_id_symbol] = async_hooks.newUid();
Copy link
Contributor

@refack refack Jul 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

So as we wrote in #14386 (comment)
This might not be enough. If the socket is being reused, then it might hold a socket[async_id_symbol] but it might be stale.
[I'm not completely awake yet, but] you might want to test socket[async_id_symbol] != socket._handle.getAsyncId()

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 think that is a completely different problem, this is built to be a stop-gap fixes that avoid crashing node 8 everywhere during unit tests.
The line below sets the type to 'not a socket', which is probably not correct in that case.

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 an issue I've been wresting with for the last month while I was working on #14208. The problem with assigning an async_id to the OutgoingMessage is because OutgoingMessage isn't actually asynchronous in any way, so it will never emit a before()/after()/destroy(). So we're only assigning an async_id as a stop gap.

There are two long term solutions that I could implement quickly:

  1. Assign the this[async_id_symbol] = newUid() in the OutgoingMessage constructor. Then use allow passing custom async_id to node::AsyncWrap::AsyncWrap #14208 to propagate that value to the socket when it's assigned. This will require detecting if the socket will call the destroy() callback, or if it needs to be queued to be called in the 'finish' event. For this to happen I'll need sign-off on said PR.

  2. I've also considered and experimented with assigning error codes to the async_id_symbol/trigger_id_symbol that can be safely handled. In this case I'd assign UV_ENODATA in the constructor since the socket hasn't been created yet, then the async_id_symbol can be assigned when the socket has been created. This has the drawback of breaking async call graphs, but is easier to implement and doesn't require the additional PR.

I'm going to step out and clear my head. Give me feedback and I can get a fix posted.

Copy link
Member Author

Choose a reason for hiding this comment

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

@trevnorris I really like 1, and I do not really understand 2. I would prefer releasing something asap as a stop-gap solution (maybe today or tomorrow), and work on a proper fix. #14208 seems a semver-minor change, but maybe I am misreading it.

Do you think that will fix #14368 as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 on (2)
IMHO if we can placate all the assertion while async_hooks are disabled that would be best.

Copy link
Contributor

Choose a reason for hiding this comment

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

@refack I'll start working on (2). It's a quicker solution and should address a broader scope of issues.

@mcollina (1) is a more long term solution for more specific cases and will probably require more testing to make sure it's done correctly. I'll get on that once the referenced PR lands. (2) is a way to mitigate these errors and allow bad values to flow through without crashing the process while allowing users of async_hooks to know when bad values appear

async_hooks.emitInit(socket[async_id_symbol], 'not-a-socket',
async_hooks.initTriggerId(), this);
}
}
});

Object.defineProperty(OutgoingMessage.prototype, '_headers', {
get: function() {
Expand Down
32 changes: 32 additions & 0 deletions test/parallel/test-http-outgoing-message-inheritance.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
'use strict';

const common = require('../common');
const { OutgoingMessage } = require('http');
const { Writable } = require('stream');
const assert = require('assert');

// check that OutgoingMessage can be used without a proper Socket
Copy link
Member

Choose a reason for hiding this comment

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

Can you link to the issues in the test files? I think that would be helpful for people looking at these tests at a later point.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

// Fixes: https://github.com/nodejs/node/issues/14386
// Fixes: https://github.com/nodejs/node/issues/14381

class Response extends OutgoingMessage {
constructor() {
super({ method: 'GET', httpVersionMajor: 1, httpVersionMinor: 1 });
}

_implicitHeader() {}
}

const res = new Response();
const ws = new Writable({
write: common.mustCall((chunk, encoding, callback) => {
assert(chunk.toString().match(/hello world/));
setImmediate(callback);
})
});

res.socket = ws;
ws._httpMessage = res;
res.connection = ws;

res.end('hello world');
27 changes: 27 additions & 0 deletions test/parallel/test-http-server-response-standalone.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
'use strict';

const common = require('../common');
const { ServerResponse } = require('http');
const { Writable } = require('stream');
const assert = require('assert');

// check that ServerResponse can be used without a proper Socket
// Fixes: https://github.com/nodejs/node/issues/14386
// Fixes: https://github.com/nodejs/node/issues/14381

const res = new ServerResponse({
method: 'GET',
httpVersionMajor: 1,
httpVersionMinor: 1
});

const ws = new Writable({
write: common.mustCall((chunk, encoding, callback) => {
assert(chunk.toString().match(/hello world/));
setImmediate(callback);
})
});

res.assignSocket(ws);

res.end('hello world');