Enhancement: inject JS stack into errors originating from native code

[mykola-mokhnach]

Feature request

What are you trying to achieve?

The current error message thrown for unsupported/broken images (Error: Input buffer contains unsupported image format) would be more helpful if it had the list of supported image formats in it. Also, Node.js shows no stacktrace for such errors.

When you searched for similar feature requests, what did you find that might be related?

Simply init sharp with some random data:

import sharp from 'sharp';

const buf = Buffer.from('12345');
try {
  const img = sharp(buf);
} catch (err) {
  console.log(err.message);  // Input buffer contains unsupported image format
  console.log(err.stack); // Error: Input buffer contains unsupported image format
}

What would you expect the API to look like?

The error message could look like: Input buffer contains an unsupported image format. Only the following image formats are supported: PNG, JPEG, WEBP, so users could easily fix the input data themselves without checking additional documentation.
As far as I can see the exception itself is thrown inside of native code. Not sure if something special needs to be done for it, so a proper stacktrace is shown (btw I run Node.js 16@macOS 13.3.1).

What alternatives have you considered?

See above

Please provide sample image(s) that help explain this feature

See above

[lovell]

This error is part of a class of errors that are thrown asynchronously from within C++ code where there is no access to the current JS stack.

To support a stack on errors (which would include everything up to and including the toBuffer or toFile call), we would probably have to wrap all native calls and, when there's an error from C++, generate a second "empty" error in JS and copy its Error().stack property to the first (or copy the message the other way).

As for which formats are supported, we should probably direct people towards inspecting sharp.format themselves.

[mykola-mokhnach]

This error is part of a class of errors that are thrown asynchronously from within C++ code where there is no access to the current JS stack.

should I then replace all calls of const img = sharp(buf) with const img = await sharp(buf)? Otherwise the whole node process will exit due to an unhandled async exception.

[mykola-mokhnach]

As for which formats are supported, we should probably direct people towards inspecting sharp.format themselves.

It does not really matter to which API we direct users in the error message. Main point there is something in this message, which helps to fix the actual error.

[lovell]

should I then replace all calls of const img = sharp(buf) with const img = await sharp(buf)? Otherwise the whole node process will exit due to an unhandled async exception.

The return value of toBuffer() and toFile() (when no callback is provided) is a Promise that can be awaited. As part of this you'll probably want to add some try/catch error handling, the same as any Promise-returning call.

[mykola-mokhnach]

The return value of toBuffer() and toFile() (when no callback is provided) is a Promise that can be awaited. As part of this you'll probably want to add some try/catch error handling, the same as any Promise-returning call.

my scenario is a bit different. I don't call toBuffer or toFile immediately on the instance. For example:

const sharp = require('sharp');


async function main() {
  let img = sharp(Buffer.from('12345'));
  if (1) {
    img = img.resize(100);
  }
  return await img.toBuffer();
}

(async () => await main())();

This code crashes node with unhandled exception:

node:internal/process/promises:279
            triggerUncaughtException(err, true /* fromPromise */);
            ^

[Error: Input buffer contains unsupported image format]

[mykola-mokhnach]

Changing let img = sharp(Buffer.from('12345')) to let img = await sharp(Buffer.from('12345')) has no effect, it still throws unhandled exception

[lovell]

Input is evaluated lazily when output is requested. The constructor returns a sharp instance (and not a Promise). The call to toBuffer() returns a Promise, which is rejecting in your sample code as there's nothing to handle it.

[mykola-mokhnach]

I'm still not quite sure the exception should be passed to node's triggerUncaughtException.

Just to compare:

const sharp = require('sharp');

async function test() {
  throw new Error();
}

async function main2() {
  return await test();
}

async function main1() {
  let img = sharp(Buffer.from('12345'));
  return await img.toBuffer();
}

(async () => await main2())();

main2 prints

/Users/elf/code/test/sharp/sharptest.js:4
  throw new Error();
        ^

Error
    at test (/Users/elf/code/test/sharp/sharptest.js:4:9)
    at main2 (/Users/elf/code/test/sharp/sharptest.js:8:16)
    at /Users/elf/code/test/sharp/sharptest.js:16:20
    at Object.<anonymous> (/Users/elf/code/test/sharp/sharptest.js:16:28)
    at Module._compile (node:internal/modules/cjs/loader:1191:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1245:10)
    at Module.load (node:internal/modules/cjs/loader:1069:32)
    at Function.Module._load (node:internal/modules/cjs/loader:904:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:22:47

while main1 prints

node:internal/process/promises:279
            triggerUncaughtException(err, true /* fromPromise */);
            ^

[Error: Input buffer contains unsupported image format]

even though toBuffer is awaited.

[lovell]

You'll need to add some kind of error handling to anything that returns a Promise as await only deals with the resolve path, not the reject path.

Anyway, we've gone way beyond the original suggestion, which was to include the stack property on any errors generated, and I still think that's a great idea so let's focus on that.

[dmitriy-komarov]

If an app has multiple Sharp calls, the error message should indicate the place where there was an exception.
Currently it contains totally helpless info about a crash.

I can wrap all the Sharp calls with such try{}catch:

try {
  await someSharpCall();
} catch (e) {
   throw Error(e as any);
}

but I suppose Sharp itself should wrap a native text exception into a JS exception with a stack trace.

[RichardBradley]

To support a stack on errors (which would include everything up to and including the toBuffer or toFile call), we would probably have to wrap all native calls and, when there's an error from C++, generate a second "empty" error in JS and copy its Error().stack property to the first (or copy the message the other way).

I think this is probably a price worth paying.

If you are suggesting that this is too expensive at runtime, maybe it could be opt-out? I think most users will see an error as an exceptional case and want clear logging, even if it is slightly computationally expensive.

At present, if applications have multiple Sharp operations in a pipeline, and an error is thrown, then the maintainers cannot tell which of the operations failed, as all Sharp errors have no stack trace.

[lovell]

v0.33.0 is now available with a stack property on all Error instances either provided to callbacks or used as the rejection of a Promise.