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

Wait for other worker thread on shutdown #2484

Closed
wants to merge 1 commit into from

Conversation

PierreF
Copy link

@PierreF PierreF commented Sep 14, 2022

When worker stop due to max-lifetime, the first thread will now wait for additional thread to complet before terminating the worker process.

This fix the issue described in #1894. With this change I never get any request error nor request that don't finish.

I think this will also fix the issue seen on production described in #2480

If you want that I make this wait behind an option, just ask and I'll update this PR.

When worker stop due to max-lifetime, the first thread will now wait
for additional thread to complet before terminating the worker process.
@PierreF
Copy link
Author

PierreF commented Oct 6, 2022

Is there something missing for this PR ?

@svanoort
Copy link

Any chance of getting a review on this one? It looks like a straightforward 4-liner bugfix to a very painful issue and I think is impacting my company as well, because the symptoms line up quite well (read: prod outages with the upgrade from Python 3.9.7 to 3.9.8 -- with the problem appearing to happen in uWSGI internal code)

@xrmx ?

@@ -3718,6 +3718,10 @@ void uwsgi_ignition() {
}
}

if (uwsgi.threads > 1 && !uwsgi_instance_is_dying) {
Copy link
Contributor

Choose a reason for hiding this comment

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

#define uwsgi_instance_is_dying (uwsgi.status.gracefully_destroying || uwsgi.status.brutally_destroying)

uwsgi_instance_is_dying includes graceful shutdown.
In graceful shutdown, we should wait other thread too.
I think simple if (uwsgi.threads > 1) test is OK.

@SteveByerly
Copy link

This was fixed in #2626 which is now commit 06a2259

The fix was shipped in v2.0.25, though the version's release notes are less explicit about the change than the commits 2.0.24...2.0.25

@xrmx xrmx closed this Jun 26, 2024
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.

5 participants