ASGI refactoring attempt #1475

tomchristie · 2019-01-18T15:02:53Z

This isn't intended to be graceful at this point, rather just to make a start on getting passing tests, using an ASGI-based app, connected to an ASGI-based test client.

The application changes are intended to be the dumbest possible thing that could provide the ASGI interface, with the intention that once the test suite was passing or sufficiently passing, then refactoring towards something far more graceful could start.

There are two changes:

The Sanic class has an ASGI based __call__ interface, that calls into a dumb shim layer.
The SanicTestClient is replaced from an aiohttp client that makes actual network requests, into a requests based client, that has an adapter to make ASGI requests direct to the application.

Because the requests interface differs in places to the aiohttp client interface, there's a bit of fiddling around to ensure that API compatibility. Again, this is just the dumbest possible place to start, with a view to moving to something more graceful later.

tomchristie · 2019-01-18T15:17:11Z

Conversation: https://community.sanicframework.org/t/considering-asgi-support/70/13

sjsadowski · 2019-02-21T15:29:30Z

@tomchristie since we've got X's across the board, how would you like to handle this? New PR after more ASGI work or update this guy when you're ready?

tomchristie · 2019-02-25T08:45:50Z

Sure, so the blocker I ran into on first attempt was the test client. I figured the first thing is to switch the test client out so that it's pointing at the ASGI interface instead.

Sanic uses an aiohttp-based test client, which makes actual network requests.
Starlette has a requests-based test client, which interacts directly with ASGI.

I naively tried just swapping one out for the other, but there's enough API differences that it doesn't really fly.

Anyways, the right way to progress this next would be...

Sort out an aiohttp-based ASGI test client, by subclassing the exsiting client so that it calls into an ASGI interface rather than making actual network requests.

Once that's sorted then we'll be able to add an ASGI interface onto Sanic, and run the existing test suite against it.

I've got a few other bits on my plate at the moment, so it might take me a bit to get onto it, but defo on the TODO list.

sjsadowski · 2019-02-25T21:19:12Z

So I was digging a bit and it looks like the aiohttp folks are somewhat resistant to ASGI inclusion.

Is it time to look at overhauling testing completely to be requests based in order to make us future-ready for doing ASGI? Personally I don't have it in me to go lobby over at aiohttp to push "closed" issues.

sjsadowski · 2019-02-25T21:52:11Z

Further, this looks like it's going to be a bigger chunk of work - do we want to split this off in to a feature branch that we can work on so more people can assist? @huge-success/sanic-core-devs

tomchristie · 2019-02-26T09:30:06Z

So I was digging a bit and it looks like the aiohttp folks are somewhat resistant to ASGI inclusion.

Doesn't really matter from the POV of Sanic's Test Client. It wouldn't be in aiohttp's codebase.

Is it time to look at overhauling testing completely to be requests based in order to make us future-ready for doing ASGI? Personally I don't have it in me to go lobby over at aiohttp to push "closed" issues.

It'd make the ASGI switch easier from my POV, but I don't think it's a blocker.

(Conversly, if sanic does stay with aiohttp interface for now, then once we've made the ASGI switch, we'll get a requests-based alternative for free.)

yunstanford · 2019-04-16T22:17:26Z

Any progress here or any blocker ?

ahopkins · 2019-04-16T22:48:03Z

Yes. I am working on swapping out the test client to using requests-async. I should be able to push something either later this week or over the weekend.

tomchristie · 2019-04-17T11:38:41Z

@ahopkins Great - make sure to ping me when there's progress there, and I'll see if there's something I oughta be contributing from my side.

ahopkins · 2019-04-18T13:28:50Z

See question here: https://community.sanicframework.org/t/considering-asgi-support/70/14?u=ahopkins

ahopkins · 2019-04-22T13:44:47Z

@tomchristie I have a question about requests-async... Is ASGISession setup to handle class based ASGI apps? It appears the answer is no.

TypeError: __call__() takes 2 positional arguments but 4 were given

If I add a new method, and pass that to ASGISession as the app, it works as expected.

class Sanic:
    ...
    def __call__(self, scope):
        return ASGIApp(self, scope)

    def asgi(self, scope, receive, send):
        asgi_app = ASGIApp(self, scope)
        return asgi_app(receive, send)

It looks to me like this is something that should be addressed. But in the mean time, I will press forward with something like the above.

tomchristie · 2019-04-22T13:58:07Z

ASGISession works against the ASGI 3 interface - a single Async callable taking scope, receive, and send. This PR is a bit out of date since it targets the old double callable style. I can update this PR if that’s helpful, tho I think the first thing would be to update the underlying test client without considering ASGI. (Just making regular http calls direct to the app in the same way as the existing client)

ahopkins · 2019-04-22T14:20:54Z

Makes sense, and that is basically what I am doing. But, I wanted to also test it against this work so far.

ahopkins · 2019-05-05T20:53:05Z

@tomchristie Can you let me know if this is something you plan to pick up again with, or if you want me to continue on? Again, I really appreciate the work you have put in so far.

tomchristie · 2019-05-06T09:26:35Z

Both options possible. I’ll try to take a look over where things are at in the coming days.

yunstanford · 2019-05-10T04:21:12Z

any timeline on this ?

ahopkins · 2019-05-10T05:34:38Z

I'm not able to put time on this this weekend. Bird my thought was next week to put some effort in to try and make it before the end of this month so we at least have it as an option for 19.6.

ahopkins · 2019-05-21T16:34:10Z

Not ready for review yet. But progress nonetheless ...

harshanarayana · 2019-06-04T17:19:11Z

@ahopkins anything I can do to help?

sjsadowski · 2019-06-05T12:15:23Z

Just as a note here: https://community.sanicframework.org/t/june-19-6-0-release/310 I was suggesting on Jun-15, but I think as long as we get it out this month (before Jun-30) we're still following the release pattern.

ahopkins · 2019-06-05T15:38:10Z

To complete:

Docs
Test coverage on asgi.py
Fixing the minor linting issues (it seems black in the Makefile is not formatting as flake8 is requiring in tox, maybe a separate issue worth looking at?)

yunstanford

I've not fully reviewed yet, but looks good overall.
Seems like still missing documentation around this, can we add that ? I'm thinking we probably should just mention ASGI support still in Beta for 19.6 release.

sanic/testing.py

yunstanford · 2019-06-12T05:32:36Z

sanic/server.py

@@ -477,7 +477,7 @@ def write_response(self, response):
    async def drain(self):
        await self._not_paused.wait()

-    def push_data(self, data):
+    async def push_data(self, data):


why do we add async here ?

I'll go back and look again. I don't remember exactly, but I do remember there was a reason. I'll add a comment.

The ASGI equivalent needs to be awaitable. Therefore, I opted to make the Sanic version also awaitable to reduce complexity. The alternative would be either adding an isawaitable check, or a try/except. I am up for changing it, but I thought the following in the StreamingHTTPResponse was the simplest.

if self.chunked: await self.protocol.push_data(b"%x\r\n%b\r\n" % (len(data), data)) else: await self.protocol.push_data(data)

sanic/asgi.py

ahopkins · 2019-06-15T19:41:30Z

@yunstanford

Seems like still missing documentation around this, can we add that ? I'm thinking we probably should just mention ASGI support still in Beta for 19.6 release.

Yes, I did write some, but I don't know what happened to it. I will write again and push it. I agree with you. That's actually what I was suggesting we do. I put a section in deployment and then said that ASGI was in beta for now.

ahopkins

I think we are ready to merge. Anyone else? @yunstanford? @sjsadowski?

I'll try to add some more testing coverage.

remove self-approval

yunstanford · 2019-06-18T17:08:57Z

Sounds good, I'll take a look tonight. Thanks!

ahopkins · 2019-06-18T17:36:57Z

Getting there. Just a little more test coverage and codecov should be happy.

I will push this later and then rebase the commits that I can.

Beautify

ahopkins · 2019-06-18T21:29:20Z

Uh.... weird that the CI is failing:

Beautify Specify websockets version

ahopkins · 2019-06-18T21:45:43Z

Ahh.... websockets 6.0 conflict with uvicorn which is requestion 7.0. Specifying it in tox. We need to tackle that one soon. I will put it on my radar for 19.9

yunstanford · 2019-06-20T23:33:11Z

Thanks, I'll prepare the release.

ahopkins · 2019-06-21T08:10:18Z

Thanks @tomchristie for your help in pushing this forward. I hope this helps the overall Python community as we all continue to build a richer set of tools.

tomchristie · 2019-06-21T09:09:56Z

@yunstanford @ahopkins - Great work folks!

harshanarayana · 2019-06-21T13:50:07Z

@ahopkins This is wonderful. Thank you so much @tomchristie Thanks

ASGI refactoring attempt

95526a8

jstriebel mentioned this pull request Feb 26, 2019

Sanic compatibility? encode/uvicorn#3

Closed

ahopkins mentioned this pull request Apr 7, 2019

WIP: ASGI Integration #1265

Closed

ahopkins changed the title ~~ASGI refactoring attempt~~ WIP: ASGI refactoring attempt Apr 18, 2019

ahopkins mentioned this pull request Apr 23, 2019

Testing client #1562

Merged

tomchristie closed this May 6, 2019

tomchristie reopened this May 6, 2019

Merge branch 'master' into asgi-refactor-attempt

c685231

Merge branch 'master' into asgi-refactor-attempt

4767a67

ahopkins mentioned this pull request May 15, 2019

Testing fails with ModuleNotFoundError: No module named 'aiohttp' #1577

Closed

Create SanicASGITestClient and refactor ASGI methods

8a56da8

Prepare initial websocket support

7b8e362

Run black and manually break up some text lines to correct linting

5f9e985

ahopkins added 2 commits June 11, 2019 11:11

Merge branch 'master' into asgi-refactor-attempt

b2d4132

Resolve linting issues with imports

ab706dd

yunstanford reviewed Jun 15, 2019

View reviewed changes

Add ASGI documentation

fb61834

ahopkins previously approved these changes Jun 18, 2019

View reviewed changes

ahopkins requested review from ahopkins and yunstanford June 18, 2019 08:51

Increase testing coverage for ASGI

62e0e5b

Beautify

ahopkins added 2 commits June 19, 2019 00:38

Increase testing coverage for ASGI

b1c23fd

Beautify Specify websockets version

Specify websockets version

3f47fa9

ahopkins removed their request for review June 18, 2019 22:11

ahopkins mentioned this pull request Jun 19, 2019

Routing is incorrect with some special characters (e.g. dot) #1525

Closed

yunstanford approved these changes Jun 20, 2019

View reviewed changes

yunstanford merged commit 891f99d into sanic-org:master Jun 20, 2019

tomchristie deleted the asgi-refactor-attempt branch June 21, 2019 09:07

ASGI refactoring attempt #1475

ASGI refactoring attempt #1475

Conversation

tomchristie commented Jan 18, 2019

tomchristie commented Jan 18, 2019

sjsadowski commented Feb 21, 2019

tomchristie commented Feb 25, 2019

sjsadowski commented Feb 25, 2019

sjsadowski commented Feb 25, 2019

tomchristie commented Feb 26, 2019

yunstanford commented Apr 16, 2019

ahopkins commented Apr 16, 2019

tomchristie commented Apr 17, 2019

ahopkins commented Apr 18, 2019

ahopkins commented Apr 22, 2019

tomchristie commented Apr 22, 2019

ahopkins commented Apr 22, 2019

ahopkins commented May 5, 2019

tomchristie commented May 6, 2019

yunstanford commented May 10, 2019

ahopkins commented May 10, 2019

ahopkins commented May 21, 2019

harshanarayana commented Jun 4, 2019

sjsadowski commented Jun 5, 2019

ahopkins commented Jun 5, 2019

yunstanford left a comment

Choose a reason for hiding this comment

yunstanford Jun 12, 2019

Choose a reason for hiding this comment

ahopkins Jun 15, 2019

Choose a reason for hiding this comment

ahopkins Jun 18, 2019

Choose a reason for hiding this comment

ahopkins commented Jun 15, 2019 • edited Loading

ahopkins left a comment • edited Loading

Choose a reason for hiding this comment

yunstanford commented Jun 18, 2019

ahopkins commented Jun 18, 2019 • edited Loading

ahopkins commented Jun 18, 2019

ahopkins commented Jun 18, 2019

yunstanford commented Jun 20, 2019

ahopkins commented Jun 21, 2019

tomchristie commented Jun 21, 2019

harshanarayana commented Jun 21, 2019

ahopkins commented Jun 15, 2019 •

edited

Loading

ahopkins left a comment •

edited

Loading

ahopkins commented Jun 18, 2019 •

edited

Loading