timeouts break when time changes

[Kristof Provost]

nginx uses gettimeofday() to obtain the current time for use with timers.

That's wrong, because gettimeofday() doesn't supply monotonic time. It should be using clock_gettime(CLOCK_MONOTONIC) instead.

For example, on one device nginx is a front for php fastgci. Timeouts are configured (fastcgi_send_timeout 300;). One PHP page allows the user to change the system time.
If the time is changed backwards everything's OK, if it's changed forwards nginx returns '504 Gateway timeout'.

The problem was detected on 1.0.5, but from a quick look at the source it is still present.

I'll try to attach a rough patch (working on Linux, but not suitable for inclusion as-is).

[Maxim Dounin]

While clock_gettime(CLOCK_MONOTINIC) is certainly better choice for timeouts handling (or, rather, CLOCK_MONOTONIC_FAST), as of now nginx uses ngx_gettimeofday() for both timeouts and real time. The clock_gettime(CLOCK_MONOTONIC) is not suitable as a replacement for real time as it counts time from an unspecified point in the past, and hence the patch is clearly wrong (even if not considering portability problems).

Use of clock_gettime(CLOCK_MONTONIC) will basically require introduction of an additional time (monotonic one) within nginx and switching to it uses which require monotonic time. Not really sure it worth the effort.

[Kristof Provost]

I checked the uses of ngx_gettimeofday() in ngix 1.0.5 (core). Everyone there uses it for timeout, not real time. It's possible there are external modules which use it that way of course.

Perhaps it'd be better to introduce ngx_clock_gettime(). In any case, my immediate problem is fixed. I just wanted to bring this issue to your attention.

[Maxim Dounin]

Take a look at ngx_time_update() in src/core/ngx_times.c. The ngx_gettimeofday() function is used as the only source of the time there, for both ngx_current_msec (which is then used mostly for timers) and cached_time (and various text representations) which is a real time. Introducing monotonic clocks for ngx_current_msec should be possible, though I don't yet see how this may be done without an extra syscall at least in common case.

Please also note that changing system time by no means is a risky operation. Even if there are monotonic clock available in a system (which is optional as per POSIX, btw) and all timeouts in the system are handled with monotonic clocks - it won't be possible to fix things like wrong filesystem dates. In particular, make(1) is known to be confused by this (and this can't be fixed).

[Kristof Provost]

Thanks, I missed that one. The additional syscall may be an issue on highly loaded systems. It's certainly something to consider carefully in nginx in general.

I'm lucky enough to only have to care about one target though and being the webUI of an embedded device it's never going to be expected to handle thousands of hits per second ;)
I'm also aware of the potential issues with make, but again, the embedded target isn't expected to be a useful compilation server.

[regnarg@…]

Added another patch (monotonic_timers.patch) against current master. This one (0) doesn't fiddle with ngx_gettimeofday, which is used at too many places, instead only ngx_event_timer is changed to use monotonic time, (1) adds an option "monotonic_timers" (default off) that allows you to configure whether you want to sacrifice the extra clock_gettime syscall, (2) adds proper #if-s and configure tests for clock_gettime and clock_monotonic, (3) passes the test suite (with the addition of some custom tests for the timeout mechanism; can upload those too, if desired).

It should be more-or-less ready to be applied, modulo stylistic issues. We ran into the same issue as the original reporter: we have a web interface for an embedded product / virtual appliance and want to have an option to set the system time from the webadmin. Currently we have to patch nginx to accomplish that.

[Maxim Dounin]

I don't think that introducing another global variable is needed. Just changing ngx_current_msec to use clock_gettime() should be enough.

Note that ngx_current_msec overflows every 48 days on 32-bit platforms, and therefore it's already expected to be "since an unspecified point in the past" as POSIX say ​about CLOCK_MONOTONIC.

Note well that CLOCK_MONOTONIC_FAST (and CLOCK_MONOTONIC_COARSE on Linux) support is desired. It's expected to be much faster than CLOCK_MONOTONIC if available. Simple benchmark on FreeBSD shows:

gettimeofday: 2.299 us
clock_gettime(CLOCK_REALTIME): 2.266 us
clock_gettime(CLOCK_REALTIME_FAST): 0.377 us
clock_gettime(CLOCK_MONOTONIC): 2.303 us
clock_gettime(CLOCK_MONOTONIC_FAST): 0.396 us

And similar one on Linux:

gettimeofday: 1.451 us
clock_gettime(CLOCK_REALTIME): 1.470 us
clock_gettime(CLOCK_REALTIME_COARSE): 0.012 us
clock_gettime(CLOCK_MONOTONIC): 1.487 us
clock_gettime(CLOCK_MONOTONIC_COARSE): 0.008 us

[regnarg@…]

second version of the patch -- overwrites ngx_current_msec instead of additional variable, supports CLOCK_MONOTONIC_(FAST|COARSE)

[regnarg@…]

Another version, which also checks the availability of each clock at runtime

[Maxim Dounin]

See also #989.

[bachp@…]

We also ran into this issue. Is there any update on this?

[Maxim Dounin]

See also #1308.

[flixr@…]

I also ran into this. To fix it I slightly updated the patch and built it for Ubuntu: ​https://launchpad.net/~flixr/+archive/ubuntu/nginx/+packages

Anything still needed here to get this patch included?

[flixr@…]

fixed previous patch

[MatthiasMichel@…]

We have successfully tested this on our embedded devices. What is needed to bring this upstream?

[Maxim Dounin <mdounin@…>]

In 7222:81fae70d6cb8/nginx:

Core: ngx_current_msec now uses monotonic time if available.

When clock_gettime(CLOCK_MONOTONIC) (or faster variants, _FAST on FreeBSD,
and _COARSE on Linux) is available, we now use it for ngx_current_msec.
This should improve handling of timers if system time changes (ticket #189).

[Maxim Dounin]

I've committed a simple patch to use clock_gettime(CLOCK_MONOTONIC) or faster variants if available. Thanks to all involved for prodding this.