Opened 12 years ago
Closed 7 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)
Change History (19)
by , 12 years ago
Attachment: | nginx-gettimeofday.patch added |
---|
comment:1 by , 12 years ago
comment:2 by , 12 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 , 12 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 , 12 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 , 9 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 , 9 years ago
Attachment: | monotonic_timers.patch added |
---|
comment:6 by , 9 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 , 9 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 , 9 years ago
Attachment: | monotonic_timers-3.patch added |
---|
Another version, which also checks the availability of each clock at runtime
comment:7 by , 9 years ago
Status: | new → accepted |
---|
comment:11 by , 7 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?
comment:12 by , 7 years ago
We have successfully tested this on our embedded devices. What is needed to bring this upstream?
comment:14 by , 7 years ago
Resolution: | → fixed |
---|---|
Status: | accepted → closed |
I've committed a simple patch to use clock_gettime(CLOCK_MONOTONIC)
or faster variants if available. Thanks to all involved for prodding this.
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.