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 #1965: About gunicorn [CRITICAL] Worker Timeout #1967

Merged
merged 3 commits into from Dec 28, 2023

Conversation

skytoup
Copy link
Contributor

@skytoup skytoup commented Jan 23, 2019

Fixes #1965

Refer to your suggestions, use time.monotonic and os.utime to fix system time changes cause the woker timeout

gunicorn/workers/workertmp.py Outdated Show resolved Hide resolved
@tilgovi
Copy link
Collaborator

tilgovi commented Jan 23, 2019

Thanks! One small change and it should be great.

@tilgovi
Copy link
Collaborator

tilgovi commented Jan 23, 2019

It would also be great to know if this solves your original issue!

@benoitc
Copy link
Owner

benoitc commented Jan 24, 2019

@skytoup does it solve your issue?

Copy link
Owner

@benoitc benoitc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the fchmod stuff still needs to be fixed. Can you change it?

Copy link
Collaborator

@tilgovi tilgovi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I approve too quickly. 😅

Still need to change self._tmp.name back to self._tmp.fileno().

@benoitc
Copy link
Owner

benoitc commented Jan 29, 2019

@skytoup we are doing a new release this week and it would be useful to include such change now, have you any questions about the proposed changes?

@skytoup
Copy link
Contributor Author

skytoup commented Jan 30, 2019

@skytoup we are doing a new release this week and it would be useful to include such change now, have you any questions about the proposed changes?

I did the modified time test in the test environment and found no problem temporarily.

However, I observed the previous timeout error on my server and found that timeout still occurs every day.

Timeout may be caused by other reasons every day, but the reason has not been found yet.

I have changed a machine to deploy the server. Timeout does not occur every day. It may be caused by other factors such as the machine environment.

@skytoup
Copy link
Contributor Author

skytoup commented Feb 8, 2019

I found the reason. Disk error due to system backup and affected gunicorn to modify the file information.

@tilgovi
Copy link
Collaborator

tilgovi commented Feb 11, 2019

Do we still want to merge this PR? It makes sense if it works and is portable.

@benoitc
Copy link
Owner

benoitc commented Feb 11, 2019

@tilgovi the idea is still interresting. Provided we replace os.utime by os.fchmod since we need to work on the file descriptor.

@tilgovi
Copy link
Collaborator

tilgovi commented Feb 17, 2019

@benoitc os.utime accepts a file descriptor.

gunicorn/workers/workertmp.py Outdated Show resolved Hide resolved
Co-Authored-By: skytoup <875766917@qq.com>
@tilgovi tilgovi self-assigned this Feb 17, 2019
@tkc17
Copy link

tkc17 commented Mar 4, 2021

Hi,

I am facing the same issue, any plans when/if this will be merged? We are running a service during boot and time sync is causing premature/false timeouts.

@tkc17
Copy link

tkc17 commented Apr 23, 2021

I can confirm that the fix works on 18.04, the test case which was failing always passed. It would be good to get this merged.

@tilgovi
Copy link
Collaborator

tilgovi commented Sep 19, 2021

@benoitc can you take another look at this please? I think it's correct. The change to using utime instead of fchmod and checking st_mtime instead of st_ctime is fine and maybe more portable to Windows filesystems in the future. If the resolution concerns us, we can use the ns option of utime and check st_mtime_ns (though many filesystems won't be that accurate).

@kylemanna
Copy link

Ran into this issue on an embedded device that doesn't have a RTC so the time is always wrong on a start-up and jumps when it's connected to the Internet. Applied this patch and the issue seems resolved.

Any updates on getting this merged, would love to see it incorporated?

@flixr
Copy link

flixr commented Oct 9, 2023

Bumping this, would be great to get this merged to finally resolve this issue.
See also #1709

@tilgovi
Copy link
Collaborator

tilgovi commented Dec 28, 2023

Superseded by #3095.

@tilgovi tilgovi closed this Dec 28, 2023
@tilgovi
Copy link
Collaborator

tilgovi commented Dec 28, 2023

Actually, #3095 does not use the monotonic trick. Feel free to reopen and rebase and we can merge this to fix the other issues.

@tilgovi tilgovi reopened this Dec 28, 2023
@tilgovi
Copy link
Collaborator

tilgovi commented Dec 28, 2023

I'll just merge, resolving conflicts. This looks goood.

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

Successfully merging this pull request may close these issues.

About gunicorn [CRITICAL] Worker Timeout
7 participants