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

Unable to grab references to uncaught exceptions #215

Open
ajjindal opened this issue Sep 15, 2020 · 28 comments
Open

Unable to grab references to uncaught exceptions #215

ajjindal opened this issue Sep 15, 2020 · 28 comments
Labels
docs Documentation request enhancement P3 question Further information is requested

Comments

@ajjindal
Copy link

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
Copy link
Contributor

grant commented Sep 15, 2020

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
Copy link
Author

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
Copy link
Contributor

grant commented Sep 15, 2020

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.

@grant grant self-assigned this Sep 15, 2020
@grant grant added docs Documentation request feature request question Further information is requested labels Sep 15, 2020
@ajjindal
Copy link
Author

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

Yes, that + onunhandledrejection would be perfect.

@grant
Copy link
Contributor

grant commented Sep 16, 2020

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
Copy link
Contributor

steren commented Sep 16, 2020

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
Copy link
Author

ajjindal commented Sep 16, 2020

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

@steren
Copy link
Contributor

steren commented Sep 16, 2020

process.on can be used today in your SDK.

@ajjindal
Copy link
Author

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

@ajjindal
Copy link
Author

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
Copy link
Contributor

grant commented Sep 24, 2020

@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
Copy link
Author

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

@grant grant closed this as completed Sep 24, 2020
@ajjindal
Copy link
Author

ajjindal commented Oct 1, 2020

@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:

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 grant reopened this Oct 1, 2020
@grant
Copy link
Contributor

grant commented Oct 1, 2020

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
Copy link
Author

ajjindal commented Oct 1, 2020

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

@marshall-lee
Copy link

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
Copy link

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
Copy link
Contributor

grant commented Oct 1, 2020

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
Copy link
Contributor

grant commented Oct 9, 2020

Re: #215 (comment)

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

@ajjindal
Copy link
Author

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

@marshall-lee
Copy link

@grant @ajjindal

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

@ajjindal
Copy link
Author

@grant let us know if this approach will work.

@grant
Copy link
Contributor

grant commented Oct 21, 2020

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
Copy link

marshall-lee commented Oct 21, 2020

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
Copy link

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
Copy link

@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
Copy link
Contributor

grant commented Oct 21, 2020

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

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:

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
Copy link
Contributor

grant commented Oct 21, 2020

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();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation request enhancement P3 question Further information is requested
Projects
None yet
Development

No branches or pull requests

6 participants