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

fix worker will die if the master exit #529

Merged
merged 2 commits into from Jan 5, 2022

Conversation

harlentan
Copy link
Contributor

No description provided.

@lamweili
Copy link
Contributor

lamweili commented Jan 4, 2022

Similar to #1089, it's due to the way EventEmitter works.

If there is no listener to the 'error' event, the EventEmitter additionally throws the error, which in turn becomes uncaughtException that terminates the Node.js process. This portion has been overlooked and this PR patches it.

Events

Error events

When an error occurs within an EventEmitter instance, the typical action is for an 'error' event to be emitted. These are treated as special cases within Node.js.

If an EventEmitter does not have at least one listener registered for the 'error' event, and an 'error' event is emitted, the error is thrown, a stack trace is printed, and the Node.js process exits.

const myEmitter = new MyEmitter();
myEmitter.emit('error', new Error('whoops!'));
// Throws and crashes Node.js

To guard against crashing the Node.js process the domain module can be used. (Note, however, that the domain module is deprecated.)

As a best practice, listeners should always be added for the 'error' events.

const myEmitter = new MyEmitter();
myEmitter.on('error', (err) => {
  console.error('whoops! there was an error');
});
myEmitter.emit('error', new Error('whoops!'));
// Prints: whoops! there was an error

(src: https://nodejs.org/docs/latest/api/events.html#error-events)

Copy link
Contributor

@lamweili lamweili left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to follow #1089 and flush the buffer on 'error' before it calls 'close'.

lib/appenders/multiprocess.js Outdated Show resolved Hide resolved
@lamweili lamweili added this to the 6.4.0 milestone Jan 5, 2022
@lamweili lamweili self-requested a review January 5, 2022 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

If save the log as a file, a disk failure will cause the node to crash.
2 participants