-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
8.2.0 crashes test when using shot #14386
Comments
Looks like it didn't resolve the JS stack correctly. I'm suspecting that
contains the critical information. |
It would be awesome if you could reproduce it as a standard test case. I can't work on it today and @refack is debugging another |
Any direction on how to get that information from |
Not sure. I only started using llnode a week ago :p Are you using |
@AndreasMadsen that does not work either. |
Assuming llnode is installed properly, edit: and start it like this: |
Thanks @bnoordhuis.
|
https://github.com/nodejs/node/blob/master/lib/_http_server.js#L157-L165 needs to attach a new asyncId if one was not there. Should I do that with |
I only have time for a quick look. I think socket[async_id_symbol] = async_hooks.newUid();
async_hooks.emitInit(socket[async_id_symbol], type /* not sure, maybe just invent a new value */,
async_hooks.initTriggerId(), socket); edit: and |
[Also just from a quick look] if this is actually a "reuse" of a previous socket maybe |
Do we have a reproduction snippet? (even with 3rd party dependencies) |
@refack this is done just over a plain stream. It's not a socket at all. PR is on the way:
'use strict'
const common = require('../common')
const { ServerResponse } = require('http')
const { Writable } = require('stream')
const assert = require('assert')
// check that ServerResponse can be inherited correctly
class Response extends ServerResponse {
constructor() {
super({ method: 'GET', httpVersionMajor: 1, httpVersionMinor: 1 });
}
}
const res = new Response()
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'); |
Windows is not being helpful 😞: D:\code\3party\shot>node8.2 node_modules\lab\bin\lab -a code -t 100 -L
C:\bin\dev\node\node8.2.exe: c:\ws\src\env-inl.h:131: Assertion `(trigger_id) >= (0)' failed. |
Just putting some pieces together, please correct me if I'm wrong:
|
If it is a reuse and |
OutgoingMessage needs a async-hooks enabled socket to work, but we support also basic streams. This PR init the async-hooks bits for the passed stream if it is needed. PR-URL: nodejs#14389 Fixes: nodejs#14386 Fixes: nodejs#14381
Isn't this the same as #14381 ? |
@mscdex I think so, yes. |
Stacktrace as described by in #14381 (comment):
lldb output:
Let me know if you need anything more.
cc @nodejs/async_hooks
The text was updated successfully, but these errors were encountered: