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

Call Application.startup in GunicornWebWorker #1105

Merged

Conversation

f0t0n
Copy link
Contributor

@f0t0n f0t0n commented Aug 22, 2016

What do these changes do?

  • Introduced the same behavior as in web.run_app() to call Application.startup() along with the request handler.
  • Extended test case to check worker.wsgi.startup has been called once.

Are there changes in behavior for the user?

None

Related issue number

#1092

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes

@asvetlov
Copy link
Member

Please upgrade to master.

appveyor blames:

=================================== ERRORS ====================================
____________________ ERROR collecting tests/test_worker.py ____________________
Using @pytest.skip outside a test (e.g. as a test function decorator) is not allowed. Use @pytest.mark.skip or @pytest.mark.skipif instead.

But the master has no pytest.skip anymore in test_worker.py

@f0t0n
Copy link
Contributor Author

f0t0n commented Aug 22, 2016

It did at #1103 as well. The branch is rebased and is up to date.

@f0t0n f0t0n force-pushed the feature/gunicorn-web-worker-on-startup branch from 4d14d10 to bf708c6 Compare August 23, 2016 20:54
@codecov-io
Copy link

codecov-io commented Aug 23, 2016

Current coverage is 98.06% (diff: 100%)

Merging #1105 into master will increase coverage by <.01%

@@             master      #1105   diff @@
==========================================
  Files            28         28          
  Lines          6364       6365     +1   
  Methods           0          0          
  Messages          0          0          
  Branches       1070       1070          
==========================================
+ Hits           6241       6242     +1   
  Misses           64         64          
  Partials         59         59          

Powered by Codecov. Last update ec83698...d90bbbe

@f0t0n f0t0n force-pushed the feature/gunicorn-web-worker-on-startup branch from bf708c6 to 545a494 Compare August 23, 2016 21:59
@@ -34,7 +34,9 @@ def run(self):
self._runner = ensure_future(self._run(), loop=self.loop)

try:
self.loop.run_until_complete(self._runner)
self.loop.run_until_complete(asyncio.gather(self._runner,
Copy link
Member

Choose a reason for hiding this comment

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

I suspect parallel execution by gather is not desired.
At least it works different that web.run_app().
Better to wait for self.wsgi.startup() and than execute self._runner for opening server-side socket etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I get you right. Do you mean to just schedule self.wsgi.startup() with ensure_future() and keep the rest as it was done before?

ensure_future(self.wsgi.startup(), loop=self.loop)
self._runner = ensure_future(self._run(), loop=self.loop)

try:
    self.loop.run_until_complete(self._runner)
finally:
    self.loop.close()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed as you suggested.

- Similar call as done before for `web.run_app`
- Extended test case to check `worker.wsgi.startup` has been called once.
@asvetlov
Copy link
Member

Every future worth to be awaited :)

self.loop.run_until_complete(self.wsgi.startup())
self._runner = ensure_future(self._run(), loop=self.loop)

try:
    self.loop.run_until_complete(self._runner)
finally:
    self.loop.close()

Initialize app on first step and run it on the second.
Wait for end of finalization process before starting the app.

@f0t0n
Copy link
Contributor Author

f0t0n commented Aug 24, 2016

Ah, I already thought to yield from self.wsgi.startup() it in self._run(), let's keep your version.

@f0t0n f0t0n force-pushed the feature/gunicorn-web-worker-on-startup branch from 545a494 to d90bbbe Compare August 24, 2016 08:47
@asvetlov asvetlov merged commit af15f57 into aio-libs:master Aug 24, 2016
@asvetlov
Copy link
Member

Thanks!

@f0t0n f0t0n deleted the feature/gunicorn-web-worker-on-startup branch August 24, 2016 11:01
@lock
Copy link

lock bot commented Oct 28, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants