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: on newer nodejs versions listening to socket errors are required #1089

Merged

Conversation

jhonatanTeixeira
Copy link
Contributor

No description provided.

@lamweili
Copy link
Contributor

lamweili commented Jan 2, 2022

From the original code snippet, it is using net.Socket.

    socket = net.createConnection(config.loggerPort || 5000, config.loggerHost || 'localhost');

net.createConnection()

A factory function, which creates a new net.Socket, immediately initiates connection with socket.connect(), then returns the net.Socket that starts the connection.

(src: https://nodejs.org/dist/latest-v16.x/docs/api/net.html#netcreateconnection)


Based on net.Socket API:

Event: 'error'

Added in: v0.1.90

Emitted when an error occurs. The 'close' event will be called directly following this event.

(src: https://nodejs.org/dist/latest-v16.x/docs/api/net.html#class-netsocket)


It seems like the behavior did not change and that the 'close' event will still get called.

@jhonatanTeixeira Would you happen to have any references that newer NodeJS requires listening to socket errors?
Nevertheless, it seems good to flush the buffer on 'error' before it calls 'close'.

@jhonatanTeixeira
Copy link
Contributor Author

Just run it on a newer nodejs and try to connect to a tcp destination like logstash for example and you will see it trhows an error. Newer nodejs require all errors thrown inside a promise to be catched, so that cancels the connection

@lamweili
Copy link
Contributor

lamweili commented Jan 4, 2022

@jhonatanTeixeira I verified. Good find!

It's due to the way EventEmitter works (not Promises in this case).

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)

@lamweili lamweili added this to the 6.4.0 milestone Jan 5, 2022
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
2 participants