Brooooooklyn / canvas

High performance skia binding to Node.js. Zero system dependencies and pure npm packages without any postinstall scripts nor node-gyp.
https://vercel.skia.rs
MIT License
1.78k stars 76 forks source link

`loadImage()` does not handle exceptions properly, causing calling code to crash ("floating" async code) #865

Closed lostfictions closed 3 months ago

lostfictions commented 3 months ago

Hi, thanks for your work on this library!

I've been hitting an odd issue occasionally and it took me a while to diagnose. I have some code that looks like this...

try {
  const image = await loadImage(someUrl);
  // do some other things, call `ctx.drawImage()`, etc...
} catch (error) {
  console.error(error);
  // contextually handle the error...
}

However, occasionally this would cause errors that would "escape" the try-catch block, crashing the program. Something like this:

TypeError: Invalid URL
    at new URL (node:internal/url:797:36)
    at ClientRequest.<anonymous> (/[project folder]/node_modules/.pnpm/@napi-rs+canvas@0.1.44/node_modules/@napi-rs/canvas/load-image.js:68:30)
    at Object.onceWrapper (node:events:634:26)
    at ClientRequest.emit (node:events:519:28)
    at ClientRequest.emit (node:domain:488:12)
    at HTTPParser.parserOnIncomingClient [as onIncoming] (node:_http_client:698:27)
    at HTTPParser.parserOnHeadersComplete (node:_http_common:119:17)
    at TLSSocket.socketOnData (node:_http_client:540:22)
    at TLSSocket.emit (node:events:519:28)
    at TLSSocket.emit (node:domain:488:12) {
  code: 'ERR_INVALID_URL',
  input: '/Images/somepicture.png'
}

I think there's two parts to this problem:

First, the reason this specific error arises is that sometimes redirects are relative and don't contain the full URL origin. So https://something.com/mypic.png might have a redirect to /pictures/mypic.png, which should be interpreted as https://something.com/pictures/mypic.png. But makeRequest() is constructing URLs without passing the base parameter, causing any relative redirects to throw an error on this line:

https://github.com/Brooooooklyn/canvas/blob/df9a942ea5b20eff2f8b4bcbfd74a6a8c603f07a/load-image.js#L66

However, there's also a second, broader problem: this error can't be caught by user code! It's located within a callback, and fails to catch the error and pass it to the reject() handler that makeRequest() receives. (In other words, it's sort of like a "floating promise", which is why none of the user's files appear within the stack trace above.)

There may be more types of errors within this kind of code that would similarly escape try...catch blocks (or Promise .catch() handlers) as well; I think the solution here is to add try...catch blocks in makeRequest().