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

Revert "Add test for uvicorn worker (#631)" #658

Merged
merged 1 commit into from
Apr 29, 2020

Conversation

euri10
Copy link
Member

@euri10 euri10 commented Apr 29, 2020

This reverts commit 44e7deb

I now think the previous behaviour was correct, and I've been confused by the use of the same timeout word in different contexts.

Add to that the fact that as rightly written in the gunicorn doc, the -timeout gunicorn flag purpose in the async case is not the same as in the sync case, it's only used to notify the Arbitrer that workers are still alive.

To sum up the flow : when using gunicorn with the -k flag you basically spawn an Arbitrer, which spawns workers (UvicornWorker in our case), workers who need to notify the arbitrer before the -timeout flag which defaults to 30s, if not they reboot.

Now to do that, the UvicornWorker inherits the timeout from the gunicorn base Worker, and this timeout is de facto set at half the -timeout value by the Arbitrer when it spawns its workers, I was missing that part when I thought the gunicorn config was not passed down :

    def spawn_worker(self):
        self.worker_age += 1
        worker = self.worker_class(self.worker_age, self.pid, self.LISTENERS,
                                   self.app, self.timeout / 2.0,
                                   self.cfg, self.log)

so as soon as a UvicornWorker is initialized, it has its notification timeout set to that half and should we change the -timeout gunicorn flag it would be correctly reflected in the UvicornWorker configuration.

@tomchristie
Copy link
Member

Gotcha, thanks.

We may also want to revert #646 and then issue a release? (Then we can take a second pass a figuring out a more precise method for allowing different reload files to be specified)

@euri10 euri10 merged commit 434d52d into encode:master Apr 29, 2020
@euri10 euri10 deleted the gunicorn_worker_reboot branch April 30, 2020 11:57
@br3ndonland br3ndonland mentioned this pull request Jun 4, 2023
3 tasks
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.

2 participants