Unable to grab references to uncaught exceptions

[ajjindal]

I work at Sentry, an open source software for application monitoring. We are adding support for Node+Cloud Functions to automatically report code errors. However, we are not able to get reference to the handler in order to capture uncaught exceptions. developers are required to manually catch the error and report it (https://github.com/GoogleCloudPlatform/stackdriver-errors-js#usage).

Here's current Sentry Node integration on Cloud Functions (https://docs.sentry.io/platforms/node/guides/gcp-functions/). This requires captureException call.

As a result of conversation with Steren and Vinod, posting the issue here.
cc: @steren @grant

[grant]

Hi @ajjindal, I'm not able to reproduce the issue.

Can you provide a clear, minimal setup for reproducing this issue?

For example, couldn't you just wrap your function in a try/catch? Are you saying the framework itself is throwing an exception that you want to catch?

---

For creating a global exception handler, I believe we could use Express middleware to register a global exception handler to all routes.

I have a blogpost with using middleware:
https://medium.com/google-cloud/express-routing-with-google-cloud-functions-36fb55885c68

Let me know how to help, or what you're looking for specifically.

[ajjindal]

To clarify, this is not a bug but more like an ask to support handling of uncaught exceptions through third party like Sentry.

I am looking for a pointer to the handler to be able to grab uncaught exceptions and report them into Sentry.
Steps to reproduce:

• Create a cloud function using Nodejs and make it throw an error
• Instrument Sentry (https://docs.sentry.io/platforms/node/guides/gcp-functions/) or any other error handling tool for that matter
• As you execute the function, the error won't be reported unless the exception is specifically reported from the catch block.

Gave more details to Steren and Vinod.

[grant]

OK, thanks for some more details. Would it be okay to catch uncaught exceptions with the Node process and handle them with Sentry?

process.on('uncaughtException', function (err) {
  Sentry.captureException(err);
  Sentry.flush(2000);
});

Or listen to before the process exits:

process.on('beforeExit', async () => {
    await something();
    process.exit(0); // if you don't close yourself this will run forever
});

https://nodejs.org/docs/latest-v12.x/api/process.html

I believe these event handlers can be sync or async functions.

[ajjindal]

Sorry for late response. I was checking on the recommendation with our tech lead.

Yes, that + onunhandledrejection would be perfect.

[grant]

Yes, that + onunhandledrejection would be perfect.

I'm not sure what you mean by this. With the above suggestion, you can catch all Node exceptions in a function handler.

What do you mean by onunhandledrejection? Where is the exception coming from?

If the exception is not coming from the Functions Framework itself, then can it be caught with the function handler?

[steren]

To be clear: We expect you to be able to use process.on('uncaughtException', function (err) {}); since this is a standard Node.js API that you can access within Cloud Functions

[ajjindal]

oh that makes sense. we use the combination in our Node SDK but your suggestion makes sense. Will love to get process.on.

[steren]

process.on can be used today in your SDK.

[ajjindal]

I must be missing something. Let me discuss internally and get back to the thread here.

[ajjindal]

So, the process.on will work. thanks for that. Our ask around 'onunhandledrejection` is for async handlers. However, if async handlers are not possible, we can skip on that ask.

Thank you for prompt responses.

[grant]

@ajjindal Is there anything else regarding grabbing uncaught exceptions that we can do?

#215 (comment) provides some code samples and links to docs.

If not, can I close this issue?

[ajjindal]

I am good for now. Please close the issue. I can come back to this if we run into issues.

[ajjindal]

@grant We are trying to integrate now and looks like the problem with uncaught exceptions still exists. We are doing process.on('uncaughtException', ...) but functions-framework-nodejs sets such handler too:

functions-framework-nodejs/src/invoker.ts

Lines 255 to 258 in 63cb628

	process.on('uncaughtException', err => {
	console.error('Uncaught exception');
	sendCrashResponse({err, res: latestRes, callback: killInstance});
	});

. This handler calls process.exit(16), so process terminates non-gracefully with some unprocessed events so Sentry fails to finish the ongoing flush() operation.

Is it possible to support graceful termination?

Can we open the issue again?

[grant]

I believe multiple event handlers are possible. It might require the function to be deployed on Node.js 12+.

Did you try beforeExit, exit, or other Node lifecycle events?

[ajjindal]

@marshall-lee can you help me with a response here?

[marshall-lee]

I believe multiple event handlers are possible

Yep, that's possible. In fact, Sentry's node integration already sets this uncaughtException handler https://github.com/getsentry/sentry-javascript/blob/8eb72865dcd62ff719441105c7eda28007a07e9d/packages/node/src/integrations/onuncaughtexception.ts#L42 and it's being called for sure.

It ends up invoking an asynchronous operation called close() which itself calls flush() (which is asynchronous too). But this operation fails to to be processed to its end because of process.exit() call right after that in another uncaughtException handler.

So basically, having this process.exit() in uncaughtException handler doesn't allow to do any final cleanup that requires asynchronous operations. process.exit() terminates system process non-gracefully with some pending events in node'js event pool.

Did you try beforeExit, exit, or other Node lifecycle events?

According to node'js docs:

The 'beforeExit' event is emitted when Node.js empties its event loop and has no additional work to schedule

When uncaught exception occurs, there could be some work in the event loop. I think this event is more suitable for normal termination.
I tried it in my helloworld app and it doesn't event fire.

As for exit event:

Listener functions must only perform synchronous operations.

So it's not suitable for calling flush().

[marshall-lee]

So what we probably need here is the ability to set some custom framework handler for uncaught exceptions. Framework would await for this handler to be executed with a reasonable timeout, and only after that it could call process.exit(16).

[grant]

I'm going to loop back with the Functions team about this.

Would a delay of 5 seconds or so before sending the HTTP response on unhandledException fix the issue? Then your additional unhandledException handler may work.

https://github.com/GoogleCloudPlatform/functions-framework-nodejs/blob/master/src/invoker.ts#L255

@marshall-lee Can you perhaps fork this repo and try to alter the framework such that the integration works on your end? If you need instructions for deploying a forked version on to Cloud Functions, let me know. You can deploy the forked version on Cloud Run more easily since you have full control of the container. See this blogpost for an example.

[grant]

Re: #215 (comment)

@ajjindal @marshall-lee Can you let us know what change you're looking for, or if this solution would work?

[ajjindal]

@marshall-lee is planning on creating a PR in the next couple days.

[marshall-lee]

@grant @ajjindal

Hi! I've drafted my idea in #229.

[ajjindal]

@grant let us know if this approach will work.

[grant]

Pardon the delay.

Having 1+ user defined async global error handlers would be fine, but I don't think there's a concrete proposed design. Can you describe a bit more how you're going to start the framework?

Questions:

• Are we starting the framework as a module? Will that work with you? This affects the design as a user just specifies their function target.
• Are you sure process events/exception handling works (process.on), or are you looking to wrap a function in a try/catch where the catch executes a user-defined function?
• Which specific Node errors are you trying to catch? Are they specific like unhandledRejection, or all errors?

---

Proposed interface

const framework = require('@google-cloud/functions-framework');

const origCallback = framework.ERROR_HANDLER.onUncaughtExceptionCallback;
framework.ERROR_HANDLER.onUncaughtExceptionCallback = (err) => {
  flush(timeout).then(() => origCallback(err));
};

Suggested interface

const framework = require('@google-cloud/functions-framework');

// Listen to https://nodejs.org/api/process.html#process_event_uncaughtexception
framework.on('uncaughtException', async (err) => {
  await flush(timeout);
};

Note: This would only solve users that use this library as a module (not a CLI/bin).

---

I was creating a prototype, but the design most common way of using this library as a CLI isn't described.

https://github.com/GoogleCloudPlatform/functions-framework-nodejs/compare/grant_custom_errors

---

It would also help me if you had an example piece of code that is expected to throw some exception so I can try things out. Here are some I created:

const process = require('process');

// Test uncaughtException
process.on('uncaughtException', () => {
  console.log('test uncaughtException');
});

// Test exit
process.on('exit', () => {
  console.log('test exit');
});
throw new Error('oh, hello there!');

// TEST SIGINT
process.on('SIGINT', () => {
  console.log('test exit');
});
let i = 0;
while (i < 100000000) {
  ++i;
}

[marshall-lee]

Are we starting the framework as a module? Will that work with you? This affects the design as a user just specifies their function target.

For now I only tested my drafted approach starting functions-framework as a binary locally on my computer. Everything worked fine. Why do you think it doesn't work with binary?

Are you sure process events/exception handling works (process.on), or are you looking to wrap a function in a try/catch where the catch executes a user-defined function?

and

Which specific Node errors are you trying to catch? Are they specific like unhandledRejection, or all errors?

With these global exception handlers I'm trying to catch any exception thrown outside of a function. Such exceptions are rare but they must be caught and sent to Sentry too.

As for exceptions thrown during execution of a function, we catch them using domain.active.on('error', ...) leveraging the fact that framework creates a domain for each incoming request. This helps us to provide a proper context to a caught exception when sending it to Sentry.

When wrapping functions, we both use try ... catch and domain.active.on('error'). The former is for synchronous errors, the latter is for asynchronous exceptions occured during function execution.

look
https://github.com/getsentry/sentry-javascript/blob/18bce3cb0c6052b61b46d245597ced345ecb14a8/packages/serverless/src/gcpfunction/http.ts#L60-L64
and
https://github.com/getsentry/sentry-javascript/blob/18bce3cb0c6052b61b46d245597ced345ecb14a8/packages/serverless/src/gcpfunction/events.ts#L41-L58

[marshall-lee]

This is the index.js file which I start simply with functions-framework --target=helloWorld:

// index.js

const Sentry = require("@sentry/serverless");
const framework = require("@google-cloud/functions-framework");

Sentry.GCPFunction.init({
  dsn: "http://123123123123123123123123123@localhost:9000/5",
  tracesSampleRate: 1.0,
});

// This piece of code must be somewhere in `Sentry.GCPFunction.init`:
const orig = framework.ERROR_HANDLER.onUncaughtExceptionCallback;
framework.ERROR_HANDLER.onUncaughtExceptionCallback = function(err) {
  console.log(`CAUGHT ${err}`);
  Sentry.captureException(err);
  Sentry.flush().then(() => {
    orig(err);
  });
}

setTimeout(() => {
  throw new Error('bad luck');
}, 10);

exports.helloWorld = Sentry.GCPFunction.wrapHttpFunction((req, res) => {
  res.send('ok');
});

[marshall-lee]

@grant I like your suggested interface more! Mine is just a PoC illustration.
Yours is also better in a sense that you could do Promise.all([sleep(timeout), ...errorPromises]) to avoid hanging of a process.

[grant]

For now I only tested my drafted approach starting functions-framework as a binary locally on my computer. Everything worked fine. Why do you think it doesn't work with binary?

I still don't have a full picture of a full sample for a function.

Thanks for the sample.

This PR #229 (comment) uses the Functions Framework as a module with require('@google-cloud/functions-framework') rather than the typical CLI we have in our docs/package json script, npx @google-cloud/functions-framework --target=helloWorld.

From the above sample, it looks like you're wishing to require the module as well.

With these global exception handlers I'm trying to catch any exception thrown outside of a function. Such exceptions are rare but they must be caught and sent to Sentry too.

OK.

As for exceptions thrown during execution of a function, we catch them using domain.active.on('error', ...) leveraging the fact that framework creates a domain for each incoming request. This helps us to provide a proper context to a caught exception when sending it to Sentry. https://github.com/getsentry/sentry-javascript/blob/18bce3cb0c6052b61b46d245597ced345ecb14a8/packages/serverless/src/gcpfunction/http.ts#L60-L64

When wrapping functions, we both use try ... catch and domain.active.on('error'). The former for synchronous exceptions and promise rejections, the latter for asynchronous exceptions occured during function execution.

Thanks.

---

I think we should implement this interface:

const framework = require('@google-cloud/functions-framework');

// Listen to https://nodejs.org/api/process.html#process_event_uncaughtexception
framework.on('uncaughtException', async (err) => {
  await flush(timeout);
};

For that to work, with the use case of I think we have to delay registering existing global error handler when this library is used as a module (i.e. required).

Functions Framework Interface

functions-framework-nodejs/src/index.ts

Lines 109 to 117 in 01b7df3

	SERVER.listen(PORT, () => {
	ERROR_HANDLER.register();
	if (process.env.NODE_ENV !== NodeEnv.PRODUCTION) {
	console.log('Serving function...');
	console.log(`Function: ${TARGET}`);
	console.log(`Signature type: ${SIGNATURE_TYPE}`);
	console.log(`URL: http://localhost:${PORT}/`);
	}
	}).setTimeout(0); // Disable automatic timeout on incoming connections.

if (require.main !== module) {
  console.log('required as a module');
  // listen to the server as normal
}
export function start() {
  // actually do listening...
}

...and user code:

const framework = require("@google-cloud/functions-framework");
framework.on('uncaughtException', (error) => { ... });
framework.start();

framework.on would amend callback functions to the global process event handlers:

functions-framework-nodejs/src/invoker.ts

Lines 254 to 281 in 01b7df3

	register() {
	process.on('uncaughtException', err => {
	console.error('Uncaught exception');
	sendCrashResponse({err, res: latestRes, callback: killInstance});
	});
	process.on('unhandledRejection', err => {
	console.error('Unhandled rejection');
	sendCrashResponse({err, res: latestRes, callback: killInstance});
	});
	process.on('exit', code => {
	sendCrashResponse({
	err: new Error(`Process exited with code ${code}`),
	res: latestRes,
	silent: code === 0,
	});
	});
	['SIGINT', 'SIGTERM'].forEach(signal => {
	process.on(signal as NodeJS.Signals, () => {
	console.log(`Received ${signal}`);
	this.server.close(() => {
	// eslint-disable-next-line no-process-exit
	process.exit();
	});
	});
	});

[grant]

The thing with starting the framework with...

const framework = require("@google-cloud/functions-framework");

...is that currently the Functions Framework registers error handlers immediately, so you cannot add custom handlers.

That's why we'd need to register handlers and start separately:

const framework = require("@google-cloud/functions-framework");
framework.on('uncaughtException', (error) => { ... });
framework.start();