errors from node:internal leak

[zuozp8]

Bug Description

When importing undici (from node_modules) after using fetch (nodejs built-in), then globalDispatcher is filled by built-in undici. Therefore using instanceof errors.UndiciError is unreliable

Reproducible By

Promise.resolve().then(async () => {
  fetch('http://httpstat.us/200') // if fetch is commented out, then it works properly

  require('net').createServer((c) => {
    setTimeout(() => c.destroy(), 100)
  }).listen(8888)

  const { request, errors } = require('undici')
  try {
    await request('http://127.0.0.1:8888/')
  } catch (err) {
    console.info(err instanceof errors.UndiciError)
  }
  process.exit(0)
})

Expected Behavior

true would be printed, as error should be instance of SocketError that inherits UndiciError

Logs & Screenshots

Environment

Checked on Node v20.13.1 and v23.5.0 using undici 7.2.0 and 5.28.4, on ubuntu 22.04 and in node:20.13-alpine3.19 container

Context

It seems natural to use instanceof, because error classes are exported. npm allows to have multiple versions of undici exported, so it's easy to have built-in undici, node_modules/undici and undici nested inside node_modules libraries.

To make it worse: ResponseStatusCodeError (which is easiest to test) is thrown outside dispatcher, so when I checked it, instanceof worked properly.

[metcoder95]

This is not the same, but seems to go under the same line as #3973.

Again, don't believe is right but the behaviour seems like expected given how the globalDispatcher setter is designed.

I'm starting to believe that we should implement a way for undici when a dispatcher is made from built-in vs node_module; otherwise this kind of problems will continue to occur.

cc: @mcollina @ronag

[zuozp8]

Sadly I don't see a clear solution. We could either:

• discourage using instanceof by deprecating errors export and exporting only error codes
  • for typescript we could export interfaces instead of classes in undici-types
• undo making dispatcher truly global
• make errors truly global
• move errors into dispatcher

[mcollina]

never use instanceof, it's already unreliable due to the nature of npm.

[zuozp8]

never use instanceof

Documentation doesn't advice against it. What would be a point of exporting Error classes from the library in the first place if developer is forbidden from using them?
I see no other DRY (typo-proof) way of checking error kind. instanceof also serves as type-guard and simplifies typescript code.

---

it's already unreliable due to the nature of npm

Duplicating dependencies is (controversial) feature of npm that aims to improve robustness. Of course it's irritating when it causes problems, but that happens in more complex projects, usually when objects are passed from one library (with own nested dependency) to another, or when errors are caught after passing though many layers.

In the example, request and errors are pulled together from the same undici module, but they already aren't compatible.

[mcollina]

Of course it's irritating when it causes problems, but that happens in more complex projects, usually when objects are passed from one library (with own nested dependency) to another, or when errors are caught after passing though many layers.

Exactly. Don't use instanceof if you don't know what is going to be thrown. Note that some test libraries (like jest) even mess with Node.js internal errors. It's really unreliable.

Documentation doesn't advice against it. What would be a point of exporting Error classes from the library in the first place if developer is forbidden from using them?

To each its own. If someone wants to use them, it have its own uses if you can guarantee it. Anyhow, every single time we are being prescriptive, people complain.

instanceof also serves as type-guard and simplifies typescript code.

That's the root cause. However this is fundamentally in conflict with the nature of npm.

---

Anyhow, if you would like to send a PR to improve the doc, it's 100% welcomed! I would love to see if there is a good solution to provide error-type safety for error codes. It's used by Node.js internals and many libraries.

[zuozp8]

I made PR to improve docs

Type safety of errors could be achieved by exporting functions that would serve as type-guards, like

const isUndiciError = (err: Error): err is UndiciError => {
  return err.code === 'UND_ERR' || err.code?.startsWith('UND_ERR')
}

const isConnectTimeoutError = (err: Error): err is ConnectTimeoutError => {
  return err.code === 'UND_ERR_CONNECT_TIMEOUT'
}