Opened 8 years ago

Closed 3 years ago

#189 closed defect (fixed)

timeouts break when time changes

Reported by: Kristof Provost Owned by: somebody
Priority: minor Milestone:
Component: nginx-core Version: 1.0.x
Keywords: timer Cc:
uname -a: Linux oslo 2.6.37-hg39c364f31fe7 #38 Thu Jul 26 11:49:33 CEST 2012 armv7l GNU/Linux
nginx -V: nginx: nginx version: nginx/1.0.5
nginx: built by gcc 4.4.5 (Ubuntu/Linaro 4.4.4-14ubuntu5)
nginx: TLS SNI support enabled
nginx: configure arguments: --without-http_rewrite_module --with-http_ssl_module --with-http_sub_module

Description

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).

Attachments (5)

nginx-gettimeofday.patch (1.6 KB ) - added by Kristof Provost 8 years ago.
monotonic_timers.patch (6.1 KB ) - added by regnarg@… 5 years ago.
monotonic_timers-2.patch (5.6 KB ) - added by regnarg@… 5 years ago.
second version of the patch -- overwrites ngx_current_msec instead of additional variable, supports CLOCK_MONOTONIC_(FAST|COARSE)
monotonic_timers-3.patch (6.3 KB ) - added by regnarg@… 5 years ago.
Another version, which also checks the availability of each clock at runtime
monotonic_timers.2.patch (6.7 KB ) - added by flixr@… 3 years ago.
fixed previous patch

Download all attachments as: .zip

Change History (19)

by Kristof Provost, 8 years ago

Attachment: nginx-gettimeofday.patch added

comment:1 by Maxim Dounin, 8 years ago

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.

comment:2 by Kristof Provost, 8 years ago

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.

comment:3 by Maxim Dounin, 8 years ago

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).

comment:4 by Kristof Provost, 8 years ago

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.

comment:5 by regnarg@…, 5 years ago

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.

by regnarg@…, 5 years ago

Attachment: monotonic_timers.patch added

comment:6 by Maxim Dounin, 5 years ago

sensitive: 0

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

by regnarg@…, 5 years ago

Attachment: monotonic_timers-2.patch added

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

by regnarg@…, 5 years ago

Attachment: monotonic_timers-3.patch added

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

comment:7 by Maxim Dounin, 4 years ago

Status: newaccepted

comment:8 by Maxim Dounin, 4 years ago

See also #989.

comment:9 by bachp@…, 3 years ago

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

comment:10 by Maxim Dounin, 3 years ago

See also #1308.

comment:11 by flixr@…, 3 years ago

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?

by flixr@…, 3 years ago

Attachment: monotonic_timers.2.patch added

fixed previous patch

comment:12 by MatthiasMichel@…, 3 years ago

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

comment:13 by Maxim Dounin <mdounin@…>, 3 years ago

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).

comment:14 by Maxim Dounin, 3 years ago

Resolution: fixed
Status: acceptedclosed

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

Note: See TracTickets for help on using tickets.