Haraka test suite crashes with #13548
Comments
refack
added
async_hooks
|
/cc @nodejs/async_hooks |
|
Using similar script as in #13325, I get the following error: Somehow the |
Any chance it's a reused socket like in #13348 ? |
|
I debugged it, this will reproduce the error: const net = require('net');
const server = net.createServer();
server.getConnections(() => {}); |
|
More explanation: when the server is created it sets the Best fix I can think of is: const asyncId = this._handle ? this[async_id_symbol] : null; |
|
Maybe we should just keep separate async ids for the JS |
I like the idea of creating a primary resource for the server and a secondary resource for |
|
Maybe investigate removing |
|
@refack I think this is a good example of why that is not as simple as the comment suggests; we need a proper async ID to pass to |
You are actually making me read the code (and stop talking out of my ****) 📜 As I see it this is an example why TypeError: Cannot read property 'getAsyncId' of undefined
at Server.prototype.getConnections (net.js:1549:14)
...which is less scary than the OP. [another bad idea] [maybe less bad] |
I think there are better ways to do that if we want to go there, such as don't assert in push/pop at all if |
Are you referring to https://github.com/nodejs/node/blob/v8.1.0/src/env-inl.h#L130-L131 and https://github.com/nodejs/node/blob/v8.1.0/src/env-inl.h#L146-L160 ? Those checks saved my sanity while developing async_hooks and I believe they should always run. The argument of "async_hooks not being used" is difficult because they can be enabled at any time, and if the stack is corrupt when that happens then the user will be getting bad information.
The question is, how would the const print = process._rawDebug;
const hook = async_hooks.createHook({
before(id) { print('>', id) },
after(id) { print('<', id) },
});
const s = net.createServer((conn) => {
print('<<', conn.getAsyncId() ,'>>');
print('<<', conn._handle.getAsyncId() ,'>>');
}).listen();
s.getAsyncId(); // 3
s._handle.getAsyncId(); // 4Here's what receiving a new connection may look like: But on the My solution would be do something simple like: diff --git a/lib/net.js b/lib/net.js
index 67c231e..1a2c54a 100644
--- a/lib/net.js
+++ b/lib/net.js
@@ -183,7 +183,7 @@ function Socket(options) {
// Problem with this is that users can supply their own handle, that may not
// have _handle.getAsyncId(). In this case an[async_id_symbol] should
// probably be supplied by async_hooks.
- this[async_id_symbol] = -1;
+ this[async_id_symbol] = 0;
this._hadError = false;
this._handle = null;
this._parent = null;
@@ -1186,7 +1186,7 @@ function Server(options, connectionListener) {
configurable: true, enumerable: false
});
- this[async_id_symbol] = -1;
+ this[async_id_symbol] = 0;
this._handle = null;
this._usingSlaves = false;
this._slaves = [];Reason is because I originally set Thought: Allow /cc @AndreasMadsen @addaleax @refack If everyone thinks being able to pass an existing |
I had the same thought. :) I think that would be the best approach, yes. |
I think we might also need that if we are going to completely fix #13427 |
|
Also the second half of #13045 has come up (http Agent non-cohesive API) |
|
If this needs to be discussed further, I think we should use |
I'm working on the patch now, but running into some issues (which should be expected by now).
I believe that approach would be an appropriate short term fix. |
|
Anything I can do to help with this? |
|
@baudehlo Yes, you can make a pull request that adds: const asyncId = this._handle ? this[async_id_symbol] : null;
nextTick(asyncId cb, err, connections);here https://github.com/nodejs/node/blob/master/lib/net.js#L1561. And add a test case with: const net = require('net');
const server = net.createServer();
server.getConnections(() => { /* test return value */ }); |
|
@AndreasMadsen does nextTick(null, cb, err, connections);work? On the one hand I remember it should, on the other I vaguely remember it throwing in #13839 |
|
That is how it is supposed to work https://github.com/nodejs/node/blob/master/lib/internal/process/next_tick.js#L300 In reality, it is a bit more flexible https://github.com/nodejs/node/blob/master/lib/async_hooks.js#L322 |
|
But what is https://github.com/nodejs/node/blob/master/lib/async_hooks.js#L323L324 about? If it is an attempt to make |
|
You should consult with the person who wrote it |
|
@refack that commit just renamed the variable, https://github.com/nodejs/node/blame/448c4c62d2b413226dfdef03d6f8d243de0984a3/lib/async_hooks.js#L311-L312 is the |
I know, just wanted to make a funny 🤣 Good call on adding actually informative context! |
I have no idea, and it looks incorrect. It's assigning what was probably an object to a number. So never mind, it's definitely incorrect. |
Fixed in #14026 |
If the .listen() hasn't been called on the server, there is no handle object. In this case use null as the triggerAsyncId. Fixes: nodejs#13548
PR-URL: nodejs#13839 Fixes: nodejs#13045 Fixes: nodejs#13831 Refs: nodejs#13352 Refs: nodejs#13548 (comment) Reviewed-By: Trevor Norris <trev.norris@gmail.com>
PR-URL: nodejs#13839 Fixes: nodejs#13045 Fixes: nodejs#13831 Refs: nodejs#13352 Refs: nodejs#13548 (comment) Reviewed-By: Trevor Norris <trev.norris@gmail.com>
PR-URL: #13839 Fixes: #13045 Fixes: #13831 Refs: #13352 Refs: #13548 (comment) Reviewed-By: Trevor Norris <trev.norris@gmail.com>
If the .listen() hasn't been called on the server, there is no handle object. In this case use null as the triggerAsyncId. Fixes: #13548 PR-URL: #13938 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
In the case where triggerAsyncId is null it should default to the current executionAsyncId. This worked but as a side-effect the resource object was changed too. This fix also makes the null check more strict. EmitInitS is not a documented API, thus there is no reason to be flexible in its input. Ref: #13548 (comment) PR-URL: #14026 Reviewed-By: Refael Ackermann <refack@gmail.com>
PR-URL: #13839 Fixes: #13045 Fixes: #13831 Refs: #13352 Refs: #13548 (comment) Reviewed-By: Trevor Norris <trev.norris@gmail.com>
If the .listen() hasn't been called on the server, there is no handle object. In this case use null as the triggerAsyncId. Fixes: #13548 PR-URL: #13938 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
In the case where triggerAsyncId is null it should default to the current executionAsyncId. This worked but as a side-effect the resource object was changed too. This fix also makes the null check more strict. EmitInitS is not a documented API, thus there is no reason to be flexible in its input. Ref: #13548 (comment) PR-URL: #14026 Reviewed-By: Refael Ackermann <refack@gmail.com>
PR-URL: #13839 Fixes: #13045 Fixes: #13831 Refs: #13352 Refs: #13548 (comment) Reviewed-By: Trevor Norris <trev.norris@gmail.com>
If the .listen() hasn't been called on the server, there is no handle object. In this case use null as the triggerAsyncId. Fixes: #13548 PR-URL: #13938 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
In the case where triggerAsyncId is null it should default to the current executionAsyncId. This worked but as a side-effect the resource object was changed too. This fix also makes the null check more strict. EmitInitS is not a documented API, thus there is no reason to be flexible in its input. Ref: #13548 (comment) PR-URL: #14026 Reviewed-By: Refael Ackermann <refack@gmail.com>
This looks similar to #13325
Full logs can be found here: https://travis-ci.org/haraka/Haraka/jobs/239765834
Test suite exits with:
Test being run is here: https://github.com/haraka/Haraka/blob/master/tests/server.js#L93
The text was updated successfully, but these errors were encountered: