stream: save error in state

[ronag]

Useful for future PR's to resolve situations where various API's are invoked on an already errored stream.

Extracted from #34035

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[ronag]

This fails with the following:

node$make-j4&&out/Release/node--expose-gc/Users/ronagy/GitHub/nxtedition/node/test/parallel/test-gc-tls-external-memory.jsAssertionError[ERR_ASSERTION]: 0<256atTLSSocket.connect(/Users/ronagy/GitHub/nxtedition/node/test/parallel/test-gc-tls-external-memory.js:36:5)atTLSSocket.<anonymous>(/Users/ronagy/GitHub/nxtedition/node/test/common/index.js:365:15)atTLSSocket.emit(events.js:314:20)atemitErrorNT(internal/streams/destroy.js:127:8)atemitErrorCloseNT(internal/streams/destroy.js:92:3)atprocessTicksAndRejections(internal/process/task_queues.js:80:21)atrunNextTicks(internal/process/task_queues.js:62:3)atprocessImmediate(internal/timers.js:435:9){generatedMessage: false,code: 'ERR_ASSERTION',actual: false,expected: true,operator: '=='}

#34035 (comment)

Seems like a GC related bug in TLSSocket? Some reference that doesn't properly cleanup? Not sure how to resolve. Who's a good ping?

[mcollina]

lgtm

[ronag]

@nodejs/streams @addaleax

[lundibundi]

LGTM. We should probably run a benchmark just to make sure it's fine.

[ronag]

Just a note, this does not pass CI because of the GC issue. I will need help to sort that out.

[ronag]

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/631/

[ronag]

This is blocking #34035

[lundibundi]

Very weird, after looking through heap-dumps of master and this PR it looks like TLSWraps are getting kept by the immediate queue items (see onSocketCloseDestroySSL in lib/_tls_wrap.js) which are not getting cleaned up properly with this PR. (there are a bunch of connected immediate queue items on the heap but they are not contiguous, so like 3 prev-next items, then another 4 prev-next not connected to the previous ones and such)
If we change that line in onSocketCloseDestroySSL to setImmediate(destroySSL.bind(this)); it will work just fine and everything gets collected properly. Though this is not a fix for the issue, it should work as it is now, it looks like the bug is somewhere else.

[ronag]

Though this is not a fix for the issue, it should work as it is now, it looks like the bug is somewhere else.

@lundibundi Would you mind creating an issue for that? I'll apply your workaround in the meantime.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/32143/ ✅
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2399/

[ronag]

That seems to have fixed it. I'd just like a thumbs up from @addaleax before landing this.

[lpinca]

My only concern here is the increased memory usage but I guess it's ok as we are in an error, hopefully uncommon, condition.

Is it possible to add a test for #34103 (comment)?

[addaleax]

So … let’s be a bit more clear about the cause of the memory leak here:

What’s happening is that the error.stack property refers to the full stack frames of its creation. V8 does this to be able to compute error.stack lazily as a string, instead of always formatting it directly even if it isn’t used. Those stack frames in turn can refer to the actual values of variables in that stack frame.

In the test, this turns out to be somewhat catastrophic, because each error’s stack frame contains the previous TLS socket, effectively creating a singly-linked list of all TLS sockets here.

So yes, this PR does introduce this problem for a specific reason. I know this is very dependent on the test’s layout, but it’s a problem I could see arising in other situations as well.

There’s four possible paths forward that I see here:

1. Don’t land this PR, because we consider this kind of memory problem too risky.
2. Land this PR, because we think this is unlikely to happen in real-world applications, or we don’t care if it occurs.
3. Always stringify error.stack, because the small performance penalty that comes with it is worth the reduced risk of memory leaks. (I’ve confirmed that this also fixes the test with the patch below.)
4. Ask the V8 team for a solution to this. It isn’t great that memory leaks like this can occur in the first place, and it doesn’t feel right at all that Error objects can keep entire stack frames and variables from those stack frames alive indefinitely (?). The big downside is that this probably takes some time to implement.

I would personally lean towards option 3, but I think any is fine. If we do pick option 2, it probably makes more sense to amend the test code than the tls source code, though.

[diff for option 3]

diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js
index b63c8f89aec5..223e9af7bc74 100644
--- a/lib/_stream_writable.js+++ b/lib/_stream_writable.js@@ -434,12 +434,14 @@ function onwrite(stream, er) {
if (er) {
if (!state.errored) {
state.errored = er;
+ er.stack;
}
// In case of duplex streams we need to notify the readable side of the
// error.
if (stream._readableState && !stream._readableState.errored) {
stream._readableState.errored = er;
+ er.stack;
}
if (sync) {
diff --git a/lib/internal/streams/destroy.js b/lib/internal/streams/destroy.js
index 575b12f9c18b..483b6d11a196 100644
--- a/lib/internal/streams/destroy.js+++ b/lib/internal/streams/destroy.js@@ -25,6 +25,7 @@ function destroy(err, cb) {
}
if (err) {
+ err.stack;
if (w && !w.errored) {
w.errored = err;
}
@@ -61,6 +62,7 @@ function _destroy(self, err, cb) {
const w = self._writableState;
if (err) {
+ err.stack;
if (w && !w.errored) {
w.errored = err;
}
@@ -175,6 +177,7 @@ function errorOrDestroy(stream, err, sync) {
if ((r && r.autoDestroy) || (w && w.autoDestroy))
stream.destroy(err);
else if (err) {
+ err.stack;
if (w && !w.errored) {
w.errored = err;
}

[ronag]

I would personally lean towards option 3

I'm good with this. Would be nice to do 4 regardless.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/32162/

[ronag]

Is it possible to add a test for #34103 (comment)?

@lpinca done

[ronag]

I would personally lean towards option 3

@addaleax done

[addaleax]

Still LGTM

[ehmicky]

There is a downside to the following, beyond the small performance penalty.

Always stringify error.stack, because the small performance penalty that comes with it is worth the reduced risk of memory leaks. (I’ve confirmed that this also fixes the test with the patch below.)

As implemented in:

node/lib/internal/streams/destroy.js

Line 35 in 8a41d9b

err.stack;// eslint-disable-line no-unused-expressions

If error.message is modified later on (e.g. due to prepending some additional information), the change won't be reflected with error.stack. This is unfortunate because error.stack is used by util.inspect(), which is itself used by console.log().

import{Writable}from'node:stream'consterror=newError('This failed')conststream=newWritable()stream.destroy(error)stream.on('error',()=>{})error.message=`Additional info: ${error.message}`console.log(error)// This does not print 'Additional info'// Error: This failed// at file:///...// ...

I opened an issue at #51715.