Server exits with code 7 on uncaught exception before our handler is called

[omairvaiyani]

New Issue Checklist

• [x] I am not disclosing a vulnerability.
• [x] I am not just asking a question.
• [x] I have searched through existing issues.
• [x] I can reproduce the issue with the latest version of Parse Server.

Issue Description

See previously closed issue which first pointed this out, but was closed due to clerical reasons.

When an error is thrown somewhere within our request handlers, if its uncaught, ParseServer catches it and re-throws. Unfortunately, this takes away any uncaught exception handling ability from our application code itself.

See this code snippet from src/ParseServer.js

 if (!process.env.TESTING) {
      //This causes tests to spew some useless warnings, so disable in test
      /* istanbul ignore next */
      process.on('uncaughtException', err => {
        if (err.code === 'EADDRINUSE') {
          // user-friendly message for this common error
          process.stderr.write(`Unable to listen on port ${err.port}. The port is already in use.`);
          process.exit(0);
        } else {
          throw err; <-- this causes the server to crash
        }
      });
     .
     ..
    }

Node.js does allow registering multiple uncaught exception handlers, however, if you throw an error from within one of these handlers, it skips any other registered handlers and exits the application with code 7.

Steps to reproduce

We noticed this error when one of our express middlewares was setting a response header after it was returned to the client. You can reproduce this by making your own faulty middleware:

app.use((req, res, next)=> {
    const originalEnd = res.end.bind(res) as typeof res.end;

     res.end = (
      ...args: Parameters<typeof originalEnd>
    ): ReturnType<typeof originalEnd> => {
      setTimeout(() => {
        console.log('setting bad header in res.end');

        res.header('X-Some-Header', 'x');
      }, 100);

      return originalEnd(...args);
    };
    next();
});

Actual Outcome

The server crashed with exit code 7.

Expected Outcome

The uncaught exception should be handled by ParseServer, or ignored and left to the application to manage.

Environment

v6.1.0-alpha.6

Server

• Parse Server version: v6.1.0-alpha.6
• Operating system: MacOS / Linux
• Local or remote host (AWS, Azure, Google Cloud, Heroku, Digital Ocean, etc): Local and AWS ECS

Database

• System (MongoDB or Postgres): MongoDB
• Database version: 4.4
• Local or remote host (MongoDB Atlas, mLab, AWS, Azure, Google Cloud, etc): Local and MongoDB Atlas

Client

• SDK (iOS, Android, JavaScript, PHP, Unity, etc): Irrelevant
• SDK version: Irrelevant

Logs

/Users/x/Documents/GitHub/synap/common/temp/node_modules//parse-server/lib/ParseServer.js:251
          throw err;
          ^

Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client
    at new NodeError (node:internal/errors:399:5)
    at ServerResponse.setHeader (node:_http_outgoing:663:11)

# printed by our on exit handler:   
application exited with code 7

[parse-github-assistant[bot]]

Thanks for opening this issue!

• 🚀 You can help us to fix this issue faster by opening a pull request with a failing test. See our Contribution Guide for how to make a pull request, or read our New Contributor's Guide if this is your first time contributing.

[mtrezza]

Your proposed PR seems to simply not throw the error. Wouldn't that mean that uncaught errors are silenced and the server won't crash?

If I understand your log example correctly the server crashes because the headers are not sent. What would be the expected behavior to notify the developer that the headers set won't reach the client?

[dblythy]

From the Node documentation:

'uncaughtException' is a crude mechanism for exception handling intended to be used only as a last resort. The event should not be used as an equivalent to On Error Resume Next. Unhandled exceptions inherently mean that an application is in an undefined state. Attempting to resume application code without properly recovering from the exception can cause additional unforeseen and unpredictable issues.

Exceptions thrown from within the event handler will not be caught. Instead the process will exit with a non-zero exit code and the stack trace will be printed. This is to avoid infinite recursion.

Attempting to resume normally after an uncaught exception can be similar to pulling out the power cord when upgrading a computer. Nine out of ten times, nothing happens. But the tenth time, the system becomes corrupted.

The correct use of 'uncaughtException' is to perform synchronous cleanup of allocated resources (e.g. file descriptors, handles, etc) before shutting down the process. It is not safe to resume normal operation after 'uncaughtException'.

To restart a crashed application in a more reliable way, whether 'uncaughtException' is emitted or not, an external monitor should be employed in a separate process to detect application failures and recover or restart as needed.

I think the current approach of letting the server crash is the correct one.

[mtrezza]

Unfortunately, this takes away any uncaught exception handling ability from our application code itself.

Exceptions in custom app code should be handled by the developer by wrapping code into a try/catch block.

But if I understood it correctly, that's not possible with a faulty custom middleware can cause Parse Server's internal uncaughtException handler to catch and re-throw the error. The error may be recoverable and not require the Parse Server to crash, as it's not an internal Parse Server exception but an unhandled exception that the developer has no possibility to handle through custom code.

It may make sense that the specific error

Cannot set headers after they are sent to the client

should cause the server to crash, because the header is missing in the request which may lead to an incorrect response that the server should not process, because it may corrupt existing data.

However, if a developer has a middleware where they want to handle a specific error themselves, then Parse Server should probably not obstruct that. We could add an option to disable Parse Server's uncaughtException handler, with a warning about the risks of doing so.

[omairvaiyani]

Your proposed https://github.com/SynapLearning/parse-server/pull/3 seems to simply not throw the error. Wouldn't that mean that uncaught errors are silenced and the server won't crash?

We would handle the exception in our application, log the error and exit the server. The PR above is just a proposal in our fork to experiment with the idea.

We could add an option to disable Parse Server's uncaughtException handler, with a warning about the risks of doing so.

This is more or less what I was aiming for - I agree that uncaught exceptions should not be silenced and that the server should exit to avoid continuing on a corrupt state, however, this should be an option for developers to handle themselves.

In our scenario, I wasn't suggesting that headers set after the response was returned should be acceptable, more that we were unable to find this error in our logs because it was caught by Parse and re-thrown. Under most circumstances, that re-throw should be perfectly fine, however, we have a specific log formatter that transmits to our observability deck in a format that can be ingested. This error thrown bypassed our logging handlers and took us a while to recreate.

There are other reasons why developers may need to use their own uncaught exception handlers - performing cleanup being a key one; Parse's inbuilt handler takes away this ability - unless I'm missing something?

The header issue was of course a coding error by us and is now fixed after identification, but I feel an option to disable the Parse uncaught exception handler, with a recommendation to setup our own, would be a good expansion of the configuration set.

[mtrezza]

This is more or less what I was aiming for - I agree that uncaught exceptions should not be silenced and that the server should exit to avoid continuing on a corrupt state, however, this should be an option for developers to handle themselves.

I think we could add such an option that disables the built-in handler. Do you want to open a PR for this? @dblythy What do you think?

[omairvaiyani]

Before I open a PR, I think we should consider what benefit ParseServer handler provides as opposed to the Node default behaviour.

Node 14 uncaughtException

By default, Node.js handles such exceptions by printing the stack trace to stderr and exiting with code 1, overriding any previously set process.exitCode.

Node 20 uncaughtException

By default, Node.js handles such exceptions by printing the stack trace to stderr and exiting with code 1, overriding any previously set process.exitCode.

This suggests that the desired behaviour of exiting on uncaught exception is simply the default in Node. Parse server only adds the EADDRINUSE === exit 0 behaviour, which I can't see a particular reason for?

src/ParseServer.js

process.on('uncaughtException', err => {
        if (err.code === 'EADDRINUSE') {
          // user-friendly message for this common error
          process.stderr.write(`Unable to listen on port ${err.port}. The port is already in use.`);
          process.exit(0);
        } else {
          throw err; <-- this causes the server to crash
        }
      });

Here's what happens if we remove the ParseServer uncaughtException handler, and try to start the server with an in-use port:

Process exited with code 1
Uncaught Error Error: listen EADDRINUSE: address already in use :::9090
    at __node_internal_captureLargerStackTrace (internal/errors:496:5)
    at __node_internal_uvExceptionWithHostPort (internal/errors:593:12)
    at setupListenHandle (net:1868:16)
    at listenInCluster (net:1916:12)
    at Server.listen (net:2004:7)

The exit code change maybe considered breaking, so I'll await your thoughts before a PR.

[mtrezza]

Good point.

The error EADDRINUSE is not auto-resolving so that for example as soon as the port becomes unused the server would start listening on the port. The only way to recover from this error would be to auto-retry binding to the port in a loop, which Parse Server itself doesn't do currently.

However, its environment may do that. If a Node.js process exits with code 0, the process is usually not auto-restarted, because the environment (PM2, Docker, AWS Elastic Beanstalk, Google App Engine, etc.) assumes the process ran successfully and exited gracefully. An exit code 1 usually will cause the environment to restart the process. If the port is in use and we remove the handler, then Parse Server will restart in a loop. Whether that is desired or not depends on whether it can be expected that the port becomes available. The current logic seems to be implemented to handle this scenario: log error and stop server to notify the developer to free the port.

If we decide to remove the handler altogether, we'd need to consider:

• Is the CLI or any other Parse Server internal code expecting exit code 0 on this particular error to act on it?
• Changing the exit code could be a breaking change; for the environment behavior explained above.

[omairvaiyani]

Thanks @mtrezza that gives some clarity to the reasoning behind the handler, and potential consequences of its removal.

I think given the recent v6 release, I'm hesitant to introduce a potential breaking change. For now, we've refactored our application code to always register our own handler before Parse is imported and it seems to run before (instead of) the parse handler.

Unless you feel otherwise, it maybe sufficient to leave it at that and let other developers stumble upon this if they search for the issue in the future.

[mtrezza]

If we decide to remove the handler altogether because the current behavior doesn't make sense, then we could do so in Parse Server 7 at the end of the year, since you already found a workaround. If we decide to keep the handler because it's useful, then we could add a new Parse Server option that disables the handler, thereby avoiding a breaking change, so we could add that option anytime.

[omairvaiyani]

Happy with that approach - closing for now. Thanks for your input.

[mtrezza]

I've reopened this to clarify what the desired solution is. If the issue is closed no further action will be taken on that matter.

If your workaround is practical, then maybe we could add it to the docs. If it's more of a hack, or the solution can't be applied in certain scenarios, then maybe adding an option makes sense?

[omairvaiyani]

Sorry for the trigger happiness.

My solution works for us, although node discourages multiple uncaught exception handlers and rightly so as it obfuscates the true handler behind the execution order.

Now I don't mind adding it as an option, but if we can go a step further, could it be a deprecation notice to continue relying on the default handler?

My reasoning behind this is because of the re-throw within the Parse handler - this causes the server to exit with code 7 suggesting that the uncaught exception handler itself ran into a problem. Instead, an exit code of 1 should be used.

In Parse v7, we could make it a breaking change and remove the handler entirely, letting developers know that the Node default behaviour (stack trace and exit code 1) will be in place, unless they wish to handle it differently.

[mtrezza]

My solution works for us, although node discourages multiple uncaught exception handlers and rightly so as it obfuscates the true handler behind the execution order.

Since it's discouraged it seems it's best to either add an option or remove the handler.

Now I don't mind adding it as an option, but if we can go a step further, could it be a deprecation notice to continue relying on the default handler?

A deprecation notice would only be necessary if we plan to change the default behavior and want to give developers a >= 1 year notice. For small breaking changes that are easy to address we don't necessarily need to add a deprecation notice, instead we can make the change with the next major version. It seems to me that this change would fall into that category.

My reasoning behind this is because of the re-throw within the Parse handler - this causes the server to exit with code 7 suggesting that the uncaught exception handler itself ran into a problem. Instead, an exit code of 1 should be used.

Are you saying that even if make the current handler optional it should be designed differently? So it should not re-throw the error but exit the process, like:

process.stderr.write(`...`);
process.exit(1);

Is that correct?

[omairvaiyani]

@mtrezza I agree with your points re: deprecation notice, that's sensible.

Are you saying that even if make the current handler optional it should be designed differently? So it should not re-throw the error but exit the process, like: process.stderr.write(...); process.exit(1);

Yes that's right, calling process.exit(1) after printing the error is preferable because otherwise (if we throw inside the handler) you give the impression that the handler itself failed. Of course, this approach of print+exit would only be the case if you actually needed to handle something else as part of the uncaught exception flow, such as closing connections, structured logging, exiting with a code other than 1, etc. If you simply want to print the stacktrace to stderr then exit with code 1, it's fine to just let Node's default behaviour handle that and not create a handler at all.

[mtrezza]

Got it. Would you want to open a PR for this? We could merge it in Oct / Nov, when we are preparing for Parse Server 7.

[d3mchenko]

@mtrezza Hello, please tell if there is a solution to this problem? My team encountered the same problem.

[parseplatformorg]

🎉 This change has been released in version 7.0.0-alpha.19

[parseplatformorg]

🎉 This change has been released in version 7.0.0-beta.1

[parseplatformorg]

🎉 This change has been released in version 7.0.0