-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Use monotonic time for process state tracking #468
Conversation
This makes sure system time changes (e.g. via ntp) doesn't result in odd assertion failures due to processes entering the running state before being started.
This makes sure system time changes (e.g. via ntp) doesn't result in odd assertion failures due to processes entering the running state before being started. Supervisor#281 Supervisor#468 https://bugs.activestate.com/show_bug.cgi?id=104508 https://activestate.atlassian.net/browse/STO-859
Disclaimer: I'm not a supervisor developer, just trying to do a review. Looks good except the part where you use ctypes. Personally I would rather have supervisor try to import time.monotonic(), if available, or else fall back to using time.time() as usual:
If this means that the monotonic issue is fixed only on Python 3, so be it. Python 3 has been widely available for many years, it's time to stop backporting every single feature to Python 2. |
Hi! Thanks for looking; anything's better than radio silence for three months 😉 The reason I used Having said that, getting even just the Python 3 part in (assuming somebody who can merge it in makes that call - it sounds like you aren't that person?) is better than nothing, since it would at least make patching in Python 2 support easier. But it feels like amending the PR at this point without knowing that would have a better chance of landing isn't productive right now. |
Hi. I am in the same boat, have a patch that needs review, sitting there for some weeks. So I thought that the supervisor maintainers must need some help, so I am reviewing some patches to help the project. Yes, I think if only the Python 3 patch is accepted, falling back to Python 2 time.time(), it keeps supervisord running as before on Python 2, but improves the behaviour if running on Python 3. Nonetheless, perhaps you can monkey-patch the time module somewhere: |
@mook-as Thanks for your work on this. I know the original motivation for this was #281, which I have hopefully fixed in eb96a2a. It would be great if you were able to help test that the fix works. I have been thinking about this PR, and I'm a bit nervous about the The suggestion from @gjcarneiro is only use monotonic time if it is available. How about something like this in
The @mook-as @gjcarneiro What do you think? |
cc @msabramo @lukeweber You are guys are pretty active lately, what do you think about this PR? |
This makes sure system time changes (e.g. via ntp) doesn't result in odd assertion failures due to processes entering the running state before being started.
Lightly tested on Linux; the BSD code is untested (written based on
man clock_gettime
and FreeBSD headers). Unfortunately I do not know how to fix it for OSX, and suspect that falling back totime.time()
is a bad idea.While running
tox
locally seems to pass, I do not know how much to trust it.Fixes #281.