make idle_timeout configurable at runtime

[pkarumanchi9]

No description provided.

[dormando]

Thanks!

Note to self: need to audit the use of the global variable

[dormando]

Small point: missing doc/protocol.txt update for the command.

Larger point: It looks like doing this properly isn't quite as trivial as it looks.

• If started with idle_timeout 0 the bg thread never starts.
• There are a few points where bad things can happen if idle_timeout has a junk read, so it probably needs to be wrapped in a lock (conn_timeout_lock?).
• Mainly the bg thread decides how long to sleep based on a read from the idle_timeout setting, thus if it gets a junk value it may end up sleeping for a few years.
• The thread has some rate limiting code with a usleep, but is holding the lock while doing this.. Meaning reusing the lock for idle_timeout could hang a worker thread.

Probably the simplest solution is to add a mutex just for the idle_timeout value, and cut the number of times it's read directly:

• memcached.c:conn_close_idle(): lock at the top, grab the value, continue with the double check under coniditional
• memcached.c:conn_timeout_thread(): probably best to grab the idle_timeout value just under the first while loop, so it stays consistent through a whole run.
• there's stats settings output check, not sure it matters as much to lock there but should for completeness.

think that's it? the rest is startup code and should be fine.

Second problem: thread doesn't exist if idle_timeout was zero.

• There are start and stop thread funcs, but stopping the thread will hang due to that sleep.
• Proper fix: rewrite conn_timeout_thread() so that it's logically locking once per chunk and carries state. Not sure I can describe this here better than writing it myself though, sorry.
• Acceptable alternative: do not allow setting idle_timeout from or to zero. Throw an informative error and we can do more work later.

Sorry, I know this looked super easy and probably Just Works most of the time, but when it fails it'll be terrible to debug and user expectations will be broken for setting from/to 0. This is probably why it wasn't added with the original code.