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

make Sanic.create_server return an asyncio.Server #1470

Merged
merged 10 commits into from
Jan 20, 2019

Conversation

denismakogon
Copy link
Contributor

  • adding 2 new parameters to Sanic.create_server:
    • return_asyncio_server=False - defines whether there's
      a need to return an asyncio.Server or run it right away
    • asyncio_server_kwargs=None - for python 3.7 uvloop doesn't
      support all necessary features like "start_serving",
      so, in order to make sanic work well with asyncio from 3.7
      there's a need to introduce generic way for passing
      kwargs for "loop.create_server"

Closes: #1469

 - adding 2 new parameters to Sanic.create_server:
   * return_asyncio_server=False - defines whether there's
     a need to return an asyncio.Server or run it right away
   * asyncio_server_kwargs=None - for python 3.7 uvloop doesn't
     support all necessary features like "start_serving",
     so, in order to make sanic work well with asyncio from 3.7
     there's a need to introduce generic way for passing
     kwargs for "loop.create_server"

Closes: sanic-org#1469
@vltr
Copy link
Member

vltr commented Jan 15, 2019

This looks promising, I really like it. One thing, though: it should be awesome to see some more tests regarding specific asyncio_server_kwargs and perhaps make use of our tox environments, where some tests are made with and without uvloop installed, while others are made in different versions of Python. This could (and should) even be part of the documentation, while some examples to the docs are always welcome (given the time).

PS: the current tests are not passing on travis, if you can take a look, it would be awesome 😉

@codecov
Copy link

codecov bot commented Jan 15, 2019

Codecov Report

Merging #1470 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1470   +/-   ##
=======================================
  Coverage   91.45%   91.45%           
=======================================
  Files          17       17           
  Lines        1732     1732           
  Branches      329      329           
=======================================
  Hits         1584     1584           
  Misses        123      123           
  Partials       25       25
Impacted Files Coverage Δ
sanic/app.py 91.72% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 99f34c9...1473753. Read the comment docs.

@denismakogon
Copy link
Contributor Author

@vltr would you mind to review this PR, please?

@yunstanford yunstanford merged commit 9cf2e1b into sanic-org:master Jan 20, 2019
With Python 3.7 AsyncIO got major update for the following types:

- asyncio.AbstractEventLoop
- asyncio.AnstractServer

Choose a reason for hiding this comment

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

typo: AbstractServer

@cakemanny
Copy link
Contributor

Hey @denismakogon I'm trying to understand this change. I understand your original intention in #1469, but there's a few adaptations here.

specifically, could you give an example using return_asyncio_server=False i.e. the default for app.create_server()?
I get an an error in any kind of sensible way I can think to use it. Here's a useless example where the behaviour seems to have changed - is this intentional?

$ cat test.py
import asyncio
import sanic

app = sanic.Sanic()

async def main():
    server = await app.create_server()
    print(server)
    print(type(server))

asyncio.run(main())
$ python3 -m venv venv18.12
$ venv18.12/bin/pip install sanic==18.12.0
...
Successfully installed aiofiles-0.4.0 httptools-0.0.13 multidict-4.5.2 sanic-18.12.0 ujson-1.35 uvloop-0.12.2 websockets-6.0
$ python3 -m venv venv19.3
$ venv19.3/bin/pip install sanic==19.3.1
...
Successfully installed aiofiles-0.4.0 httptools-0.0.13 multidict-4.5.2 sanic-19.3.1 ujson-1.35 uvloop-0.12.2 websockets-6.0

$ venv18.12/bin/python test.py
[2019-03-27 11:42:21 +0000] [34824] [INFO] Goin' Fast @ http://127.0.0.1:8000
<Server sockets=[<uvloop.PseudoSocket fd=13, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=('127.0.0.1', 8000)>]>
<class 'uvloop.loop.Server'>

$ venv19.3/bin/python test.py
[2019-03-27 11:49:53 +0000] [35015] [INFO] Goin' Fast @ http://127.0.0.1:8000
[2019-03-27 11:49:53 +0000] [35015] [ERROR] Unable to start server
Traceback (most recent call last):
  File "/private/tmp/venv19.3/lib/python3.7/site-packages/sanic/server.py", line 745, in serve
    http_server = loop.run_until_complete(server_coroutine)
  File "uvloop/loop.pyx", line 1445, in uvloop.loop.Loop.run_until_complete
  File "uvloop/loop.pyx", line 1438, in uvloop.loop.Loop.run_until_complete
  File "uvloop/loop.pyx", line 1347, in uvloop.loop.Loop.run_forever
  File "uvloop/loop.pyx", line 452, in uvloop.loop.Loop._run
RuntimeError: Cannot run the event loop while another loop is running
Traceback (most recent call last):
  File "test.py", line 11, in <module>
    asyncio.run(main())
  File "/usr/local/Cellar/python/3.7.2_2/Frameworks/Python.framework/Versions/3.7/lib/python3.7/asyncio/runners.py", line 43, in run
    return loop.run_until_complete(main)
  File "uvloop/loop.pyx", line 1451, in uvloop.loop.Loop.run_until_complete
  File "test.py", line 7, in main
    server = await app.create_server()
  File "/private/tmp/venv19.3/lib/python3.7/site-packages/sanic/app.py", line 1209, in create_server
    asyncio_server_kwargs=asyncio_server_kwargs, **server_settings
TypeError: object NoneType can't be used in 'await' expression
sys:1: RuntimeWarning: coroutine 'Loop.create_server' was never awaited

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.

6 participants