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

fix: use a monotonic clock in memory storage #7

Merged
merged 1 commit into from
Mar 21, 2021

Conversation

xrmx
Copy link
Contributor

@xrmx xrmx commented Apr 5, 2017

That should be more robust against system time update, e.g.
ntp or DST change.

@codecov-io
Copy link

codecov-io commented Apr 5, 2017

Codecov Report

Merging #7 into master will decrease coverage by 11.23%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master       #7       +/-   ##
===========================================
- Coverage     100%   88.76%   -11.24%     
===========================================
  Files           8        8               
  Lines         207      356      +149     
===========================================
+ Hits          207      316      +109     
- Misses          0       40       +40
Impacted Files Coverage Δ
token_bucket/storage.py 100% <100%> (ø) ⬆️
token_bucket/version.py 73.5% <0%> (-26.5%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61f4e63...5039d3f. Read the comment docs.

@kgriffs
Copy link
Member

kgriffs commented Apr 21, 2017

This looks OK. monotonic() is a tiny bit slower than time(), but only by a few hundred ns, so nothing to worry about.

However, it is slow enough to slightly increase ratio_of_negatives in the test_negative_count() test. You will need to up the max used in the cert to get the test to pass again.

@xrmx
Copy link
Contributor Author

xrmx commented Apr 25, 2017

Here it also fails in tests/test_multithreading.py:144: AssertionError

>       assert target_ratio < actual_ratio < ratio_max
E       assert 0.71 < 0.62

Not sure how to fix that

@vytas7
Copy link
Member

vytas7 commented Mar 21, 2021

Hi @xrmx !
And thanks for your great work maintaining uWSGI if I'm not mistaken 🙂

Just to clarify, even the normal time.time() is not subject to DST changes, and even small ntp updates can be sometimes applied in a monotonic fashion, depending on your OS and kernel version.

It seems time.monotonic() is now available in the standard lib since 3.5+, so we could probably use that where available.

That should be more robust against system time update, e.g.
ntp or leap seconds.
@xrmx
Copy link
Contributor Author

xrmx commented Mar 21, 2021

Hi @vytas7

Python docs says it uses gettimeofday:

time() returns the most accurate time available (using Unix gettimeofday() where available)

gettimeofday man page says:

       The time returned by gettimeofday() is affected by discontinuous
       jumps in the system time (e.g., if the system administrator
       manually changes the system time).  If you need a monotonically
       increasing clock, see clock_gettime(2).

So the change still looks sensible to me, I've pushed using time.monotic() because it's clearer but CI still runs of Python 2 it'll not work. Said that I have no use for this PR so leave it for posterity :)

@vytas7
Copy link
Member

vytas7 commented Mar 21, 2021

Aha, right, good call regarding the underlying implementation... That does seem indeed susceptible to time adjustments. OTOH, DST doesn't, to my understanding, affect time.time().

I'll try to move this stuff to GH Actions, and drop 3.3 and 3.4 support.

Regarding 2.7, I guess we could also drop it since it's long past its EOL... OTOH it doesn't cost much to keep it unless a major development is planned, which seems less likely at this point.
@kgriffs thoughts?

@vytas7
Copy link
Member

vytas7 commented Mar 21, 2021

In contrast to the previously suggested 3rd party library, I'm not able to benchmark any meaningful performance difference between time.time() and time.monotonic() on modern CPythons.

So merging this 🤠

@vytas7 vytas7 merged commit 042fd6e into falconry:master Mar 21, 2021
@vytas7
Copy link
Member

vytas7 commented Jun 30, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants