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

Hangs NodeJS on shutdown when using maxAge. #25

Closed
lscpike opened this issue Nov 9, 2014 · 17 comments · May be fixed by maxiplux/swagger-ui#4, godaddy/node-flipr-etcd#5 or maxdavidson/tribus#3
Closed
Assignees

Comments

@lscpike
Copy link

lscpike commented Nov 9, 2014

NodeJs 0.10.33

When I shut down the server it hangs up to the maxAge time before the process exits. Suggests the cache expiry timer is keeping the event loop going.

If I clear the cache explicitly the process exits immediately, but this means I need to track all memoized functions.

@medikoo medikoo self-assigned this Nov 9, 2014
@medikoo
Copy link
Owner

medikoo commented Nov 9, 2014

I see the issue. How do you shut down the server? Just programmatically within a process, or do you send some signal to the process, and server shuts down, but timeouts keep it going?

@lscpike
Copy link
Author

lscpike commented Nov 9, 2014

Handling the SIGINT or SIGTERM signal for example. A debugger sends a SIGINT on stop I'm pretty sure. Basically any method that allows the process to finish processing current requests/work.

In this particular case I call the shutdown method on a custom server so it can record state and log4js so it can flush to disk.

@medikoo
Copy link
Owner

medikoo commented Nov 9, 2014

@lscpike thanks, so solution is easy, I'll make sure all timeouts are cleared on kill signal.

@lscpike
Copy link
Author

lscpike commented Nov 9, 2014

That might work fine, but I'm not sure whether the one signal is enough. A more precise solution might be to call unref on the timeoutObject returned from setTimeout. However, the docs suggest this might cause performance issues if used too often.

@iammaart
Copy link

iammaart commented Jul 6, 2015

How about making 'unreffing' of the timers created by maxAge an extra configuration option? That way the user can decide whether the potential performance penalty is worth it.

@nevercast
Copy link

I just observed this, currently I'm reducing maxAge to a time I'm willing to wait for the process to exit.

@puzrin
Copy link

puzrin commented Jan 20, 2016

Not sure that listening global kill is the best idea. You may not know how user handle additional signals like SIGHUP/SIGUSR1/SIGUSR2. For example, he may wish to do soft restart of workers in cluster. I'd vote for global method to clear all timeouts (if that possible). That can be useful when soft restart needed.

@medikoo
Copy link
Owner

medikoo commented Jan 20, 2016

@puzrin I agree, also It may be totally different case on Windows.

@puzrin
Copy link

puzrin commented Jan 23, 2016

Found: https://nodejs.org/api/timers.html#timers_unref

Timers in memoizee should be "unref-ed". That will do exactly what we need.

@medikoo
Copy link
Owner

medikoo commented Jan 25, 2016

@puzrin indeed, great found! We'll follow that then

@nevercast
Copy link

Not sure if the bug is still around but unref used to cause memory leaks.
I know the npm:odbc-pool module has a .drain() method that clears all the timeouts for use when shutting down your application. That might be some reference.

unref is likely a suitable option, as it's meant to work.

@puzrin
Copy link

puzrin commented Jan 25, 2016

In the case of setTimeout when you unref you create a separate timer that will wakeup the event loop, creating too many of these may adversely effect event loop performance -- use wisely.

Problem could be in creating too many timeouts. I noted somewhere in another issues, that timer ticks management should be done by single timer object.

@fampinheiro
Copy link

I'm currently hitting this problem. Do you settled in a solution?

@medikoo
Copy link
Owner

medikoo commented Nov 7, 2016

@fampinheiro no it's still not addressed, It will for sure be addressed for v1, but date when that happens is uncertain. PR's are welcome.

@madnight
Copy link

Same problem here under Arch Linux. The maxAge values prevents node from shutdown for the time period defined in maxAge.

@medikoo
Copy link
Owner

medikoo commented Feb 23, 2018

Fixed following advice by @puzrin from #25 (comment) (not sure why it took me so long, sorry about that).

Published as v0.4.12

@medikoo
Copy link
Owner

medikoo commented Feb 23, 2018

On additional note, with v1 there's a plan to not rely on timers at all.
Whether value is stale will be confirmed at invocation. Downside is that cache may grow in memory without control, but that can be further adjusted with max setting

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment