Prevent the engine from exiting on non-critical SMTP errors from the logger #1109
Conversation
Use the `error` event for error handling instead of the `exitOnError` option, which can also be a function to conditionally exit on certain errors. Relying on `exitOnError` introduces a 3-second delay before shutdown, during which asynchronous operations may still execute, leading to potential unintended side effects. See https://github.com/winstonjs/winston/blob/195e55c7e7fc58914ae4967ea7b832c9e0ced930/lib/winston/exception-handler.js#L222
Ndpnt
force-pushed
the
increase-logging-error-resistance
branch
from
dab92e2
to
19de91f
Compare
| @@ -51,7 +62,6 @@ if (config.get('@opentermsarchive/engine.logger.sendMailOnError')) { | |||
| ssl: true, | |||
| timeout: 30 * 1000, | |||
| formatter: args => args[Object.getOwnPropertySymbols(args)[1]], // Returns the full error message, the same visible in the console. It is referenced in the argument object with a Symbol of which we do not have the reference but we know it is the second one. | |||
| exitOnError: true, | |||
There was a problem hiding this comment.
Why didn't we simply implement the whole thing by removing this line? 🤔 😅
There was a problem hiding this comment.
If we remove this line while keeping rejectionHandlers defined, Winston will catch all uncaughtException events without exiting the process, which is not the intended behavior. By default, both Node.js and Winston terminate the process when an uncaughtException occurs, as these are considered programmer errors. And this is the correct approach. As explained here, crashing immediately is the best way to handle programmer errors.
I agree that the previous code may have given the impression that we were exiting only on SMTP logging errors, but that wasn’t actually the case. Once exitOnError is set to true, the process will exit for any type of error, not just those from a specific transport. Also, I’m not sure this option is available at the transport level—it seems to be only available at the logger level. I believe the previous configuration at the transport level was a mistake. |
Co-authored-by: Matti Schneider <matti@opentermsarchive.org>
No description provided.