Skip to content

Call Application.startup in GunicornWebWorker#1105

Merged
asvetlov merged 2 commits intoaio-libs:masterfrom
f0t0n:feature/gunicorn-web-worker-on-startup
Aug 24, 2016
Merged

Call Application.startup in GunicornWebWorker#1105
asvetlov merged 2 commits intoaio-libs:masterfrom
f0t0n:feature/gunicorn-web-worker-on-startup

Conversation

@f0t0n
Copy link
Copy Markdown
Contributor

@f0t0n f0t0n commented Aug 22, 2016 •
edited
Loading

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
Copy Markdown
Member

asvetlov commented Aug 22, 2016

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
Copy Markdown
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
Copy Markdown

codecov-io commented Aug 23, 2016 •
edited
Loading

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
asvetlov
asvetlov reviewed Aug 24, 2016
View reviewed changes
aiohttp/worker.py Outdated

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

@asvetlov asvetlov Aug 24, 2016

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
Copy Markdown
Contributor Author

@f0t0n f0t0n Aug 24, 2016

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
Copy Markdown
Contributor Author

@f0t0n f0t0n Aug 24, 2016

Choose a reason for hiding this comment

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

Fixed as you suggested.

@f0t0n
Call Application.startup in GunicornWebWorker
43656d0 
- Similar call as done before for `web.run_app`
- Extended test case to check `worker.wsgi.startup` has been called once.
@asvetlov
Copy link
Copy Markdown
Member

asvetlov commented Aug 24, 2016

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
Copy Markdown
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
Copy Markdown
Member

asvetlov commented Aug 24, 2016

Thanks!

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

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.

Reviewers

No reviews

Assignees

No one assigned

Labels

outdated

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@f0t0n @asvetlov @codecov-io