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

New multiprocess manager #2183

Merged
merged 65 commits into from
May 28, 2024
Merged

Conversation

abersheeran
Copy link
Member

@abersheeran abersheeran commented Dec 15, 2023

Summary

About #2164.

The multiprocess manager introduced by this PR includes process keep-alive and process hung detection. It also imitates gunicorn and uses hup, ttin, and ttou signals to control child processes.

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@abersheeran abersheeran requested a review from Kludex December 15, 2023 04:52
pyproject.toml Outdated
@@ -100,7 +100,7 @@ omit = [

[tool.coverage.report]
precision = 2
fail_under = 98.35
fail_under = 96.82
Copy link
Member

Choose a reason for hiding this comment

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

I'm not willing to introduce this feature without testing it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I just want to see your attitude towards this PR before adding the test. Because I remember you have implemented something similar before.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, then let me review and get back to you.

@Kludex Kludex added the waiting author Waiting for author's reply label Jan 20, 2024
Copy link
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

Checklist

  • Increase coverage for the new multiprocess manager to 100% (if tests are slow, run on a separate job in the CI, and only if the multiprocess.py is changed).
  • Docstrings.
  • Documentation (I can help here).
  • Answer the question: how this should interact with --reload?
  • Move UvicornWorker out from Uvicorn's source code (I'll do it).

uvicorn/supervisors/multiprocess.py Outdated Show resolved Hide resolved
uvicorn/supervisors/multiprocess.py Outdated Show resolved Hide resolved
uvicorn/supervisors/multiprocess.py Outdated Show resolved Hide resolved
uvicorn/supervisors/multiprocess.py Show resolved Hide resolved
@Kludex
Copy link
Member

Kludex commented Mar 2, 2024

@abersheeran I'm fully available to move this forward. 👍

@Kludex
Copy link
Member

Kludex commented Mar 3, 2024

Pinging @ahopkins to review the initial steps since he offered helped on #2164. 🙏

@Kludex
Copy link
Member

Kludex commented Mar 4, 2024

Ok. 👍

We can move forward with the checklist above 🙏

Copy link
Member

@ahopkins ahopkins left a comment

Choose a reason for hiding this comment

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

I see what the PR is doing, but I somewhat fail to understand the why and the goals. Is the pinging the ultimate feature add here?

@ahopkins ahopkins self-requested a review March 4, 2024 09:21
@Kludex Kludex added the hold Don't merge yet label Mar 4, 2024
@abersheeran
Copy link
Member Author

abersheeran commented Apr 14, 2024

Also, if I press CTRL + C on uvicorn main:app --workers 2, it doesn't work as expected.

Can you try the latest commit version? This should solve the problem.

Copy link
Contributor

@baseplate-admin baseplate-admin left a comment

Choose a reason for hiding this comment

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

Please use the English version of the documentation instead of chinese version

docs/deployment.md Outdated Show resolved Hide resolved
@Kludex Kludex mentioned this pull request May 25, 2024
@abersheeran abersheeran requested a review from Kludex May 25, 2024 15:59
@abersheeran abersheeran removed the waiting author Waiting for author's reply label May 25, 2024
@Kludex
Copy link
Member

Kludex commented May 26, 2024

I think the PID should be shown in the "uvicorn.access" logger (only when workers > 1).

Is it normal for the child processes to be alive after I kill the manager process?

@abersheeran
Copy link
Member Author

I think the PID should be shown in the "uvicorn.access" logger (only when workers > 1).

Here I just reused the previous logger. If we want to refactor the logs, I think it can be done in another PR.

Is it normal for the child processes to be alive after I kill the manager process?

No, normally only killing the manager process through the SIGKILL signal will result in the survival of child processes (this is due to system signal mechanism reasons, and it also exists in all other multi-process managers), under other circumstances, child processes should be killed.

pyproject.toml Show resolved Hide resolved
@abersheeran abersheeran requested a review from Kludex May 27, 2024 18:51
@Kludex
Copy link
Member

Kludex commented May 28, 2024

Thanks @abersheeran 🙏

@Kludex Kludex merged commit 53fa273 into encode:master May 28, 2024
15 checks passed
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