remove need to explicitly close socket

[notslang]

Duplicate of #9 (since that one can't be re-opened)

Since we aren't listening for messages, there's no need to keep it open once the rest of the script is done, and there isn't any need to close it early because UDP doesn't maintain any sort of a connection.

This requires a major version bump and the intention is to fix namshi/winston-graylog2#24

[ronkorving]

Since what version of Node is unref supported? Wasn't that a relatively new thing? I'm asking because according to package.json we're good from Node 0.6.11. Now deprecating that shouldn't be a big deal (if you're still on 0.6, you have bigger problems), but I wouldn't want to get our requirements wrong.

[ronkorving]

Also, a big concern. What does this mean to in transit log messages? Some messages consist of multiple UDP messages (sent in batches). It seems to me that by doing this, and no longer offering an asynchonous shutdown API we don't allow people to wait for all chunks to have streamed out.

[notslang]

I can trace the socket.unref() docs back to v0.9.1 (https://github.com/joyent/node/commit/bdd1a740cb5029860a2eef20d8e9814f378d99c4), but I haven't found the exact version that the functionality was added.

[notslang]

As for waiting for all the chunks to be streamed out, from what I've tested, it seems that the program won't close until all the chunks have been streamed out.

[ronkorving]

But there's no way to tell, is there? If an application uses process.exit() to close, there's no way to wait anymore for the final message to be sent. This library is for logging, and if an error causes a shutdown, a really want to receive that error in Graylog2.

[notslang]

If you call process.exit(), then I don't think you'd be able to ensure that the final messages are sent anyway... you'd only be able to do that if you set a callback for logger.close(cb) (in the current version), or in logger.client.on('drain', cb) in the version in this PR.

But if you don't have to exit right away (you can wait for a callback) then you probably shouldn't be using process.exit().

[ronkorving]

You shouldn't be using process.exit() perhaps, but as an application developer you don't always control the dependencies you use, and whether they guarantee a clean shutdown or not.

on('drain') still opens the door to unsent chunks not leaving the application. Sending multiple chunks is a sequential, asynchronous process. The socket won't know if there are still chunks waiting to be sent.

[notslang]

hmm... maybe we could emit our own drain event when it's really done? we already act kinda like a stream, so it might be worth actually exposing a stream, and just wrapping it with a few helper methods for the various log levels.

...I'll look into this

[unlucio]

Hey guys, just wondering what happened to this: it would be great to see this feature live. What about if you just allow the user to choose give an "auto close option" and just state on the doc what will happen. Differently the lib will keep behaving as it is. My 2 cents.

[unlucio]

@ronkorving are you keep to ever include this patch from @slang800 ? :)

[ronkorving]

I will happily merge it if it unequivocally improves the library. There's work to be done to make sure half-messages aren't sent, which seems incredibly important (at least to me) especially during the shutdown process. I wouldn't want some incredibly fatal error not to reach my logger.

[notslang]

We cannot (and currently do not) ensure that all messages will reach the logger... things like segfaults within native modules will still shut down nodejs before our messages are sent, and there's nothing within JS that we can do to help that.

The only remaining issue with this patch is that you cannot easily attach a handler for when all the messages have been sent (you'd need to use logger.client.on('drain', cb)). And this only matters when you need to call process.exit or something like that at the end of your program, and cannot just use process.on('exit', ...) for cleanup or process.exitCode for setting the exit code. It doesn't have anything to do with ensuring half-messages aren't sent.

[ronkorving]

The remaining issue is that process.on('uncaughtException', function (error) { graylog.emergency(error.message); }); will not work. It will not reach the send() function before Node has already shut down, because during _log (aka emergency, etc) there are asynchronous operations. Since there are no user land callbacks (it's fire and forget), there is nothing we can do about this in userland right now. This to me is not an acceptable situation.

Uncaught exceptions may be the most important errors you'll ever log. The argument that we cannot catch a segfault is a straw-man, in that it's completely unrelated to the problem I don't want this PR to cause.

Don't get me wrong: I would love for this to be solved. I think using unref vs. close is really elegant. But what I don't want is my uncaught exceptions not reaching my Graylog server.

[stelcheck]

What can we do to get this merged? Uncaught exception already doesn't get sent AFAIK, shouldn't we merge this and solve uncaught exceptions separately?

[ronkorving]

I think one improvement in dealing with this is to stop using callbacks in send(), and make it a synchronous for-loop to send all chunks. If we do that, the client cannot be closed in the middle of sending a batch.

The use-case for that callback doesn't apply to use (see docs), as we already listen for the error event, and we don't need to reuse a buffer.

Thoughts?

[unlucio]

@ronkorving I see your point but it feels like logs could end up interfering with the program's flow as side effect (ie: blocking the event loop while sending lots of logs)

[ronkorving]

I think datagrams are sent non-blocking, regardless of there being a callback or not.

[unlucio]

Mumble, I'm not sure either, but whatever needs to be done before the end of the process (even in case of crashing) need to be sync and prevent the functions stack to terminate, thus I'd be concerned about eventual locks. That's what my guts are telling me.

[ronkorving]

(sorry, I just merged a PR that immediately addresses a bug... it conflicts with this PR)

[ronkorving]

With regards to dgram callbacks, https://github.com/nodejs/node/pull/4374 may provide some more insight. I think the callback is virtually useless...

[unlucio]

I think that a PR open since 10 month (on a topic not addressed in any other PR) kind of means there's not really interest in this feature. I'd be interested in knowing how people generally prevent their apps from hanging, instead of terminating their life cycle, when a connection to graylog is open.

[ronkorving]

In my case, I close everything I can close, then process.exit(0); I would like to change that :)

[ronkorving]

FWIW, I've been working on a full rewrite of this library which solves this issue once and for all. It's coming :)