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

skipFiles does not suppress "uncaught" exceptions in web streams #53789

Open
rotu opened this issue Jul 9, 2024 · 17 comments · Fixed by #53800
Open

skipFiles does not suppress "uncaught" exceptions in web streams #53789

rotu opened this issue Jul 9, 2024 · 17 comments · Fixed by #53800
Labels
debugger Issues and PRs related to the debugger subsystem. repro-exists Issues with reproductions.

Comments

@rotu
Copy link

rotu commented Jul 9, 2024

Version

22.4.1

Platform

Microsoft Windows NT 10.0.22635.0 x64

Subsystem

No response

What steps will reproduce the bug?

Run this code in the VSCode debugger with "skipFiles":["<node_internals>/**"] in the launch configuration.

import net from 'node:net';
import stream from 'node:stream';
const socket = net.connect('http://host.invalid.');
socket.on('error', (e) => {
   console.error('Caught the error', e.message);
});
let myStream = stream.Duplex.toWeb(socket);

setTimeout(() => {
   console.log('done');
}, 1000);

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior? Why is that the expected behavior?

It is expected that the debugger won't trip. This promise rejection is entirely created and handled within node's internals and should be suppressed by the VSCode's skipFiles filter.

What do you see instead?

The debugger breaks when the promise is rejected.

The trace (given by console.trace) when it is rejected looks like:

Trace
    at eval (eval-7c2224bb.repl:1:9)
    at Object.reject (<anonymous>)
    at writableStreamRejectCloseAndClosedPromiseIfNeeded (node:internal/webstreams/writablestream:771:28)
    at writableStreamFinishErroring (node:internal/webstreams/writablestream:903:5)
    at writableStreamStartErroring (node:internal/webstreams/writablestream:755:5)
    at writableStreamDefaultControllerError (node:internal/webstreams/writablestream:1202:3)
    at WritableStreamDefaultController.error (node:internal/webstreams/writablestream:502:5)
    at Socket.<anonymous> (node:internal/webstreams/adapters:195:18)
    at Socket.<anonymous> (node:internal/util:538:20)
    at Socket.onclose (node:internal/streams/end-of-stream:152:25)

The stack on the error object is:

'AbortError: The operation was aborted
    at handleKnownInternalErrors (node:internal/webstreams/adapters:111:14)
    at Socket.<anonymous> (node:internal/webstreams/adapters:179:13)
    at Socket.<anonymous> (node:internal/util:538:20)
    at Socket.onclose (node:internal/streams/end-of-stream:152:25)
    at Socket.emit (node:events:532:35)
    at Pipe.<anonymous> (node:net:339:12)
    at Pipe.callbackTrampoline (node:internal/async_hooks:130:17)'

Screenshot 2024-07-09 133433

Additional information

No response

@rotu
Copy link
Author

rotu commented Jul 9, 2024

Related: #51093 (the debugger falsely believes promise rejections in web streams are unhandled).

@RedYetiDev
Copy link
Member

RedYetiDev commented Jul 9, 2024

I believe skipFiles is controlled by VSCode, not Node.js, though I could be mistaken.

Issue #51093 reports to a problem in the V8 Debugger, whereas this one is about an issue in VSCode. I was thinking of #53732, whoops.

@RedYetiDev RedYetiDev added the debugger Issues and PRs related to the debugger subsystem. label Jul 9, 2024
@rotu
Copy link
Author

rotu commented Jul 9, 2024

Yes, skipFiles is controlled by VSCode. And the Chrome debugger supports a similar "ignore list" feature. Adding the pattern ^node:internal does not seem to suppress the debugger. Even adding the pattern .* does not seem to suppress the debugger from breaking here!

If there's a more correct way to suppress these exceptions, please let me know!

@RedYetiDev
Copy link
Member

It sounds to me like this is a VSCode bug, and not a Node.js one, but again, I may be mistaken.

@rotu
Copy link
Author

rotu commented Jul 9, 2024

I'm not sure what the pattern is supposedly matching against when the debugger is deciding whether to pause at a breakpoint - the stack traces in play (shown above) seem to match ignore-listed scripts, so I'd expect there not to be a breakpoint hit.

It sounds to me like this is a VSCode bug, and not a Node.js one, but again, I may be mistaken.

Might be, but if so, then Chrome has the same behavior. I can't tell if there's any workaround to #53732 that allows trapping uncaught exceptions without making async code to "nuisance trip" the debugger on an eventually-handled rejection.

@RedYetiDev
Copy link
Member

@nodejs/v8 - is this a V8 debugger issue, or a VSCode/Chrome issue?

@benjamingr
Copy link
Member

The promise is rejected at writableStreamRejectCloseAndClosedPromiseIfNeeded and is then immediately handled to prevent it from having an unhandled rejection which is why Node.js doesn't report an unhandled rejection here:

  stream[kIsClosedPromise].reject(stream[kState]?.storedError);
  setPromiseHandled(stream[kIsClosedPromise].promise);

The other instances I can trigger this are similar:

  stream[kIsClosedPromise].reject(error);
  setPromiseHandled(stream[kIsClosedPromise].promise);

This indeed seems overly eager (and incorrect) on V8's side. Additionally skipFiles not working seems like a VSCode bug.

I'll open a PR with a workaround from our side until it's addressed there. Those aren't real errors.

@benjamingr
Copy link
Member

#53800

@RedYetiDev

This comment was marked as outdated.

@rotu rotu closed this as not planned Won't fix, can't repro, duplicate, stale Jul 12, 2024
@RedYetiDev RedYetiDev reopened this Jul 12, 2024
@RedYetiDev RedYetiDev closed this as not planned Won't fix, can't repro, duplicate, stale Jul 12, 2024
@RedYetiDev

This comment was marked as outdated.

@benjamingr
Copy link
Member

benjamingr commented Jul 13, 2024

This isn't closed, the fact skipFiles does not suppress exceptions in core and that it's still a caught exception is a bug and we should track it since it affects our users even if we can't fix it.

@benjamingr benjamingr reopened this Jul 13, 2024
@RedYetiDev
Copy link
Member

RedYetiDev commented Jul 13, 2024

we should track it since it affects our users even if we can't fix it

Is this being tracked anywhere that it is actionable? From what I can tell, V8's trackers show related issues as (mostly) fixed, although it appears otherwise.

@RedYetiDev RedYetiDev added the repro-exists Issues with reproductions. label Jul 13, 2024
@jasnell
Copy link
Member

jasnell commented Jul 13, 2024

FWIW, there's a v8 internal API that is likely relevant here...v8::Promise::MarkAsSilent ... https://v8docs.nodesource.com/node-22.4/d3/d8f/classv8_1_1_promise.html#a085c428f3f9600830a87819f99e9fda6 ... there is likely a performance impact but might be possible for us to expose this API to JavaScript internally.

@rotu
Copy link
Author

rotu commented Jul 14, 2024

@jasnell Interesting. I'm not sure that's going to help in this case. At least for the web streams API, these rejected promises should trip the debugger if the user awaits them (but V8 doesn't do this - it only trips the debugger when the promise becomes rejected, if at all).

@jasnell
Copy link
Member

jasnell commented Jul 15, 2024

Given the way the promises work, if I await a promise it creates a new promise... So the pattern here is:

Create P1 ... Mark P1 as handled and silent. If P1 rejects then debugger will not be triggered.

Await P1... Creates P2

When P1 rejects, P2 is rejected. P2 is not marked as handled or silent, so P2 will trigger the debugger. I think this is exactly the pattern that is needed here.

@rotu
Copy link
Author

rotu commented Jul 15, 2024

@jasnell I wish it worked that way, and I believe that is the way it works for unhandledrejection events.

The debugger, on the other hand, seems to trip based on the explicit throw or Promise.reject call from user code.

All the below examples create unhandled rejections. IMO they should all pause the debugger:

const x = Promise.reject(new Error("on purpose"))

// these trip the debugger:
const a1 = x.catch((e) => { throw e })
const a2 = x.catch((e) => { Promise.reject(e) })

// none of these trip the debugger:
const b1 = x.then()
const b2 = Promise.resolve(x)
const b3 = new Promise((resolve) => { resolve(x) })
const b4 = Promise.resolve().then(() => x)
const b5 = Promise.resolve({ then: (resolve) => resolve(x) })
const b6 = (async () => { await x })()

// none of these trip the debugger either:
const b7 = new Promise((resolve, reject) => { x.then(resolve, reject) })
const b8 = x.catch(
  (e) => ({
    then: (resolve, reject) => { reject(e) }
  })
)
const b9 = x.catch(Promise.reject.bind(Promise))

edit: even worse the above cases only report the stack trace where the Error object was created (at least with node flags set to --unhandled-rejections=warn --trace-warnings --trace-uncaught --report-uncaught-exception --async-stack-traces) and the stack trace to emitUnhandledRejectionWarning. From the output you can't tell which line is responsible for which warning! Even --trace-promises doesn't help much.

aduh95 pushed a commit that referenced this issue Jul 16, 2024
PR-URL: #53800
Fixes: #53789
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
ehsankhfr pushed a commit to ehsankhfr/node that referenced this issue Jul 18, 2024
PR-URL: nodejs#53800
Fixes: nodejs#53789
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
@benjamingr
Copy link
Member

This isn't closed, the fact skipFiles does not suppress exceptions in core and that it's still a caught exception is a bug and we should track it since it affects our users even if we can't fix it.

@benjamingr benjamingr reopened this Aug 13, 2024
marco-ippolito pushed a commit that referenced this issue Aug 19, 2024
PR-URL: #53800
Fixes: #53789
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
marco-ippolito pushed a commit that referenced this issue Aug 19, 2024
PR-URL: #53800
Fixes: #53789
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
marco-ippolito pushed a commit that referenced this issue Aug 19, 2024
PR-URL: #53800
Fixes: #53789
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debugger Issues and PRs related to the debugger subsystem. repro-exists Issues with reproductions.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants