hapijs / good

hapi process monitoring
Other
525 stars 161 forks source link

Crash when reporter throws an error #565

Closed MaaKut closed 5 years ago

MaaKut commented 7 years ago

I have a Hapi+Good configuration which reports to logstash via good-logstash-tcp. The problem is that whenever logstash instance becomes unavailable, an error is thrown by the reporter which in turn causes Good to crash the Hapi process.

Shouldn't Good handle errors like that? I can hardly imagine a situation where application crash in case of logging failure could be a desired behaviour.

arb commented 7 years ago

Yes it should. That's what these lines are for https://github.com/hapijs/good/blob/master/lib/monitor.js#L148-L151

If you can create a simple to reproduce case, I can investigate further.

AdriVanHoudt commented 7 years ago

@arb why not throw there?

arb commented 7 years ago

Because then it would crash the whole server.

MaaKut commented 7 years ago

If you can create a simple to reproduce case, I can investigate further.

Here's a code sample:

const good = require('good');

const server = new hapi.Server();

server.connection({
    port: 8081
});

server.route({
    method: 'GET',
    path: '/',
    handler: (rq, reply) => reply('ok')
});

server.register({
    register: good,
    options: {
        reporters: {
            consoleReporter: [
                { module: 'good-console' },
                'stdout'
            ],
            logstashReporter: [
                {
                    module: 'good-logstash-tcp',
                    args: [{
                        tlsOptions: {
                            host: 'localhost',
                            port: 5959
                        }
                    }]
                }
            ]
        }
    }
});

server.start(error => {
    if (error) {
        console.error('Server start failed:', error);
        throw error;
    }
    console.log(`Server is running: ${server.info.uri}...`);
});

The package.json is:

  "dependencies": {
    "good": "^7.3.0",
    "good-console": "^6.4.0",
    "good-logstash-tcp": "^0.2.1",
    "hapi": "^16.5.2"
  }
}

With this setup I get the following output:

events.js:163
      throw er; // Unhandled 'error' event
      ^

Error: Max retries reached, transport in silent mode, OFFLINE
    at Socket.<anonymous> (/.../logger-sample/node_modules/good-logstash-tcp/lib/logstash.js:178:36)
    at emitOne (events.js:96:13)
    at Socket.emit (events.js:191:7)
    at TCP._handle.close [as _onclose] (net.js:511:12)

...and the server stops

AdriVanHoudt commented 7 years ago

@arb (if you want we can discuss this elsewhere) is there a way to catch that? I'd prefer my server auto reboots so my logging works again then this weird state. What happens with the log stream, does it just voids?

arb commented 7 years ago

@MaaKut that's not really a simple server as it relies on modules outside the hapi organization. @AdriVanHoudt that makes sense, how would we handle that? Pass some kind of optional fail callback?

MaaKut commented 7 years ago

@arb The only external module here is good-logstash-tcp - it's just a reporter, whose crash brings down the whole server. Do you mean it throws an error in some proper way so that good is unable to handle it?

AdriVanHoudt commented 7 years ago

@arb either that or an option to tell it to throw, the callback would be cleaner though (so I can log the error still and then throw to crash and restart)

lock[bot] commented 4 years ago

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.