Unref poller timers to avoid keeping node.js alive

[voxpelli]

Problem solved:

Currently the timers in the pollers keeps a node.js application alive indefinitely.

I myself encountered this when running my Mocha tests as all of the sudden they wouldn't shut down on their own, as Mocha tests nowadays default to do graceful shutdowns. I pinpointed the issue toi this module and tested this specific solution.

Solution:

Node.js offers a way to indicate that a timer should not keep the application alive: .unref()

I've added these to the two pollers and this solved my issue.

Automated tests:

I've not come up with an automated way to test this, so unfortunately there are no tests added here.

Legal:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[haotianw465]

Thank you for reporting the issue and provide the fix. I agree this is an issue for testing. Timeout.unref() is an implicit approach of shutdown the sampler and might have side effects. Based on the unref documentation the process might exit when there is nothing in the event loop. My understanding on this is that the pollers can be quietly shutdown when the application is idle or any transient state where the event loop queue somehow is empty.

A better and more explicit way is to provide an API on the AWSXRay module (something like shutdownSampler) which utilize clearInterval to explicitly cleanup the timeout. The issue with this approach is that those pollers are used in default and thus timers started implicitly. It's hard to have users know a shutdown needs to be called to gracefully exit. But we definitely need this one as an option. Please let me know if you have any thoughts on this.

There is also an API to disable poller based sampling https://github.com/aws/aws-xray-sdk-node/blob/master/packages/core/lib/middleware/mw_utils.js#L121. Is this an acceptable workaround for you when you are testing your application?

[voxpelli]

@haotianw465 Thanks for the quick feedback!

unref():s only effect is that whenever a node.js script would have otherwise shut down if the timeout hadn't been there, it will with an unref() shut down rather than waiting for that timer to trigger.

As a timer can be an essential part of a script, even of the shutdown process of a script, node.js typically keeps a script alive for as long as there's eg. a timer active. The unref() indicates to node.js that the timer isn't an essential part of the script but rather something that should just be running as long as the rest of the script is running.

I added an example here: https://gist.github.com/voxpelli/f1577b75b570db4ad285d51dff9617a0

As eg. a http server keeps the script alive in itself, the pollers will continue to execute until the server is closed – no matter if there's any events in the event loop or not, as shown in my last example in that gist.

As the poller timers are started implicitly I think the correct approach is to also have them shut down implicitly and that's exactly what unref() is for 🙂

[haotianw465]

Thank you for the clarification. I have merged your PR and will roll it out with the next release.

[voxpelli]

Thanks for merging @haotianw465! Any chance for a patch release soonish?