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

On ASGI's double-callable interface. #78

Open
tomchristie opened this Issue Jan 31, 2019 · 37 comments

Comments

Projects
None yet
5 participants
@tomchristie
Copy link
Member

tomchristie commented Jan 31, 2019

This isn't intended as an actionable point right now, rather just a point of conversation. I think it's worth bringing up given that ASGI is pre-PEP at this point. (And that we might still just about be at a point where we could adapt things if we felt it worthwhile enough.)

I'm starting to wonder (again) if the double-callable structure if neccessarily a better trade-off than a single-callable. eg. the difference between:

ASGI as it stands:

class App:
    def __init__(self, scope):
        self.scope = scope

    async def __call__(self, receive, send):
        await send({
            'type': 'http.response.start',
            'status': 200,
            'headers': [
                [b'content-type', b'text/plain'],
            ]
        })
        await send({
            'type': 'http.response.body',
            'body': b'Hello, world!',
        })

An alternative interface:

async def app(scope, receive, send):
    await send({
        'type': 'http.response.start',
        'status': 200,
        'headers': [
            [b'content-type', b'text/plain'],
        ]
    })
    await send({
        'type': 'http.response.body',
        'body': b'Hello, world!',
    })

The double-callable does allow classes to directly implement the ASGI interface, and for class based endpoint implementations it ends up being simpler.

However it's more complex for function based endpoint implementations, for middleware implementation patterns, and for instantiate-to-configure application implementations.

In each of those cases you end up either with a closure around an inner function, or partially-bound functools.partial. Both of those cases make for less easy-to-understand concepts and more awkward flows.

E.g. A typical middleware pattern with ASGI...

class Middleware:
    def __init__(self, app, **middleware_config):
        self.app = app
        ...

    def __call__(self, scope):
        return functools.partial(self.asgi, scope=scope)

    async def asgi(self, receive, send, scope):
        ...

An alternative:

class Middleware:
    def __init__(self, app, **middleware_config):
        self.app = app
        ...

    async def __call__(self, scope, receive, send):
        ...

Similarly with instantiate-to-configure ASGI applications, you currently end up having to return a partially bound method:

StaticFiles as ASGI...

class StaticFiles:
    def __init__(self, directory):
        self.directory = directory
        ...

    def __call__(self, scope):
        return functools.partial(self.asgi, scope=scope)

    async def asgi(self, receive, send, scope):
        ...

Alternative:

class StaticFiles:
    def __init__(self, directory):
        self.directory = directory
        ...

    async def __call__(self, scope, receive, send):
        ...

The flip side with a single-callable is that you can't point directly at a class as an ASGI interface. For endpoint implementations, you end up needing to do something like this pattern instead...

class App:
    def __init__(self, scope, receive, send):
        self.scope = scope
        self.receive = receive
        self.send = send

    async def dispatch(self):
        await self.send({'type': 'http.response.start', ...})
        await self.send({'type': 'http.response.body', ...})

    @classmethod
    async def asgi(cls, scope, receive, send):
        instance = cls(scope, receive, send)
        await instance.dispatch()

# app = App.asgi

This all starts matters a little in Starlette, where ASGI is the interface all the way through - you end up with a more complex flow, and less easy-to-understand primitives. It also ends up with more indirect tracebacks.

(I noticed this in particular when considering how to improve the stack traces for 500 responses. I was hoping to delimit each frame as "Inside the middleware stack", "Inside the router", "Inside the endpoint" - that's easy to do if the traceback frames match up directly to the ASGI functions themselves, but it's not do-able if the frames are from a function that's been returned from another function.)

Anyways, not asking for any action on this neccessarily, but it's something I've been thinking over that I wanted to make visible while ASGI is still pre-PEP.

@tomchristie

This comment has been minimized.

Copy link
Member Author

tomchristie commented Jan 31, 2019

Closing since I'm not expecting actionable responses on asgiref as a result of this, but certainly open to any discussion here.

@tomchristie

This comment has been minimized.

Copy link
Member Author

tomchristie commented Jan 31, 2019

/cc @pgjones

@davidism

This comment has been minimized.

Copy link

davidism commented Jan 31, 2019

This is going to be one of the tricky things with making Flask compatible with ASGI. Init is for creating the app, not starting the scope, so we'd need to figure out some way to make call do double duty to push the scope then handle the request.

@tomchristie

This comment has been minimized.

Copy link
Member Author

tomchristie commented Jan 31, 2019

Thanks @davidism, that's helpful feedback.

Init is for creating the app, not starting the scope

Same in Starlette, yeah.

It's not massively problematic as it stands, it's just less direct:

ASGI:

class Flask:
    def __init__(self, **config):
        ...

    def __call__(self, scope):
        return functools.partial(self.asgi, scope=scope)

    async def asgi(self, receive, send, scope):
        ...

Single callable:

class Flask:
    def __init__(self, **config):
        ...

    async def __call__(self, scope, receive, send):
        ...

(Obviously there's a whole different set of things to consider wrt. how best to keep both ASGI and WSGI compatability, but that's a different question altogether.)

@andrewgodwin

This comment has been minimized.

Copy link
Member

andrewgodwin commented Feb 1, 2019

I forget my exact motivation for having the double-call method these days, though I do remember I seriously weighed both before picking the current layout.

I think in the end it was down to wanting to have an "instance" to pass around the server that I could then use as a key and assign event loops and attributes to, which is a bad case of letting implementation define specification. I don't think there's inherently anything totally necessary about having double-call as a mechanic, though I do agree now is maybe the last chance to change it.

Let's talk about feasibility here - if it were to change, we still need to maintain some sense of backwards compatibility for servers (frameworks/apps I think are a bit more free to just say "use the newest server versions"). Without attributes on the application object, or use of inspect, is there any reasonable way to know an app is one type over another?

Also, we could totally ship an adapter from single- to double-call in asgiref, like this:

def single_to_double(app):
    class double:
        def __init__(self, scope):
            self.scope = scope
        def __call__(self, receive, send):
            app(scope, receive, send)
    return double
@tomchristie

This comment has been minimized.

Copy link
Member Author

tomchristie commented Feb 1, 2019

Also, we could totally ship an adapter from single- to double-call in asgiref.

I'd not really considered that aspect of it - yeah that's a good point.

Presumably we'd be able to use asyncio.iscoroutinefunction(func) to differentiate between the two cases reliably.

@pgjones

This comment has been minimized.

Copy link
Contributor

pgjones commented Feb 1, 2019

I see this as similar to React's functional and class components, which basically consist of either

async function App(props) {
  return ...;
}

// or

class App extends React.Component {
  constructor(props) {
    ...
  }

  async render() {
    const state = this.state;
    return ...
  }
}

depending on whether you have state or not to worry about. I think then ASGI could follow suite and allow,

async def app(scope, receive, send):
    ...

# or

class App:
    def __init__(self, scope): ...

    async def __call__(self, receive, send): ...

The key then is to know (server side) which invocation to use. React does this in a quite clever way which boils down too,

// Inside React
class Component {}
Component.prototype.isReactComponent = {};

// We can check it like this
class Greeting extends Component {}
console.log(Greeting.prototype.isReactComponent); // ✅ Yes

The simplest thing is probably to just insist that ASGI class interfaces do something similar,

class App:
    is_asgi_class = True

    def __init__(self, scope): ...

    async def __call__(self, receive, send): ...

# or

class Special:
    # Note no is_asgi_class
    async def __call__(self, scope, receive, send):
        ...

So I think it is worthwhile to change the specification from double callable to class or functional applications. I think this as the functional form is quite clear to use and we can document it as React does (state or not). I've also not seen much use of a double functional callable in practice, so removing it (although you could attach is_asgi_class to the function object) should be ok.

I can't think of a reliable way to make this backwards compatible though.

@andrewgodwin

This comment has been minimized.

Copy link
Member

andrewgodwin commented Feb 1, 2019

i feel we could use asyncio.iscoroutinefunction to provide backwards compatibility here - if it's not a coroutine function, then do a double-callable, and if it is, do a single-callable with (scope, receive, send). We could also allow override attributes as @pgjones suggests to force the other way.

@pgjones

This comment has been minimized.

Copy link
Contributor

pgjones commented Feb 1, 2019

I don't think asyncio.iscoroutinefunction would identify the Special case above though.

@andrewgodwin

This comment has been minimized.

Copy link
Member

andrewgodwin commented Feb 1, 2019

Just checked and you're right, but we could also check that and the object's __call__ directly, as that works:

>>> asyncio.iscoroutinefunction(Special())         
False                                              
>>> asyncio.iscoroutinefunction(Special().__call__)
True                                               

It would need a bit more intelligence about is-this-a-class than that, but it would work.

@pgjones

This comment has been minimized.

Copy link
Contributor

pgjones commented Feb 1, 2019

I went down this route and couldn't convince myself there was a reliable way; considering the App above Special, asyncio.iscoroutinefunction(App().__call__) is True yet it should be called differently.

@andrewgodwin

This comment has been minimized.

Copy link
Member

andrewgodwin commented Feb 1, 2019

Well remember, you'll get passed the class for double-call apps like App and an instance for something single-call like Special (as it would not be valid otherwise as it's not async callable). So I figure:

  • If it's an uninstantiated class, call in double-call mode (as class constructors cannot be async)
  • If it's an instantiated class, test to see if __call__ is a coroutine function and decide based on that
  • If it's not a class at all, test to see if it's a coroutine function and decide based on that

This doesn't have to be totally perfect, just capture about 95% of use cases and sensibly explode in the other 5% of cases so people can implement a quick fix for their apps.

@pgjones

This comment has been minimized.

Copy link
Contributor

pgjones commented Feb 2, 2019

This seems to work, see this. It seems like it there should be a simpler way though.

@tomchristie

This comment has been minimized.

Copy link
Member Author

tomchristie commented Feb 2, 2019

Not had a chance to test it, but maybe this?...

asyncio.iscoroutinefunction(getattr(app, “__call__”, app))
@tomchristie

This comment has been minimized.

Copy link
Member Author

tomchristie commented Feb 2, 2019

Oh okay, probably not. Eg. Checks the wrong thing in the case of an uninstatiated class.

@andrewgodwin

This comment has been minimized.

Copy link
Member

andrewgodwin commented Feb 5, 2019

Right, but I believe we can still check if it's an instantiated class or not and then do a check.

So, given that this appears to be feasible to do in a backwards-compatible manner, I want to ask if we think it's sensible to make the shift. Specifically, should we:

a) Allow for both kinds of application (single- and double-call) natively
b) Only allow the existing (double-call) apps but ship a single-to-double adapter in asgiref
c) Move to only allowing single-call natively and use the backwards-compat solution above to allow for a transitionary period for servers

@tomchristie

This comment has been minimized.

Copy link
Member Author

tomchristie commented Feb 5, 2019

Well, I think it's either (d) "leave things just as they are" or (c) "Move to single-call, with a compat period".

Of those (c) would very likely be my prefered option: I think the knock-on effects of having a simpler call-flow would be a big deal if ASGI does end up with very large-scale adoption.

I would probably implement the single-callable in a Starlette branch as a demonstration first, before taking a definitive stance.

Minorish aside: We've currently have something of a hack for servers to determine if the lifespan protocol is supported by an application or not, in that servers differentiate between an exception when the application is instantiated vs. an exception when the ASGI callable is invoked. That wouldn't make sense any more with a single callable. (It doesn't work reliably anyways, eg. some ASGI middleware will have good implementation reasons to only instantiate the child app within the ASGI callable, which masks which of the two cases the child app has raised an exception within.)

@davidism

This comment has been minimized.

Copy link

davidism commented Feb 5, 2019

One nice property about the __init__ + __call__ pattern is that Flask can tell whether it's in WSGI or ASGI mode automatically: if __call__ gets two args, it's WSGI, otherwise it's ASGI and should (for now) produce an intermediate scope callable. Is this a good thing to rely on? Will it even be useful when we do implement ASGI? 🤷‍♂️ It's probably better to set that in __init__ or os.environ.

@davidism

This comment has been minimized.

Copy link

davidism commented Feb 5, 2019

I think in the end it was down to wanting to have an "instance" to pass around the server that I could then use as a key and assign event loops and attributes to, which is a bad case of letting implementation define specification.

Flask already has the concept of the request context, an object that represents the request and some extra state, which gets put on a thread-local stack. My intuition would be to use the same thing in ASGI, upgrading it to be aware of Python 3.7 context locals. So the double callable would just add a level of indirection with the partial wrap.

@tomchristie

This comment has been minimized.

Copy link
Member Author

tomchristie commented Feb 6, 2019

Okay, had "Da-da-DAAAA" moment this morning.

Class instances can be awaitable.

class App:
    def __init__(self, scope, receive, send):
        self.scope = scope
        self.receive = receive
        self.send = send

    def __await__(self):
        return self.dispatch().__await__()

    async def dispatch(self):
        ...

This gives you: await App(scope, receive, send), which will instantiate the class, and then run dispatch(). So the single-callable style can still use an "instantiate then run" pattern, and can support uninstantiated classes as applications.

One nice property about the __init__ + __call__ pattern is that Flask can tell whether it's in WSGI or ASGI mode automatically

Sure, so:

class App:
    def __init__(self, **config):
        pass

    def wsgi(self, environ, start_response):
        print({"environ": environ, "start_response": start_response})

    async def asgi(self, scope, receive, send):
        print({"scope": scope, "receive": receive, "send": send})

    def __call__(self, *args):
        assert len(args) in (2, 3)
        if len(args) == 2:
            return self.wsgi(*args)
        return self.asgi(*args)


async def run():
    app = App()
    print("WSGI:")
    app("environ", "start_response")
    print("ASGI:")
    await app("scope", "receive", "send")


loop = asyncio.get_event_loop()
loop.run_until_complete(run())
@tomchristie

This comment has been minimized.

Copy link
Member Author

tomchristie commented Feb 6, 2019

Okay, didn't want this to have to be treated as an open ticket unless it was actually considered a viable option. I figure since it's actually under consideration I might as well open it up.

@tomchristie tomchristie reopened this Feb 6, 2019

@andrewgodwin

This comment has been minimized.

Copy link
Member

andrewgodwin commented Feb 15, 2019

Totally. I think this is feasible, the question I really have is how to roll it out. Do we just put the works-both-ways code into current ASGI servers and say it's done? The spec probably needs a major version bump, too?

@tomchristie

This comment has been minimized.

Copy link
Member Author

tomchristie commented Feb 18, 2019

Do we just put the works-both-ways code into current ASGI servers and say it's done?

Yes, I think so.

The spec probably needs a major version bump, too?

Yes.

Right now the server can advertise what version of the ASGI protocol it implements, and can indicate which extensions it supports. However we've got no way for the client to advertise which version it supports, and which message types it accepts. That's a problem eg. with 2->3, because servers that send lifespan messages to clients that do not accept them have no way of determining if any raised exception was due to application startup failing, or due to lifespan being unsupported.

A remedy for this, which would also help make ASGI more robust to any other incremental changes would be for the very first thing that the client does if it accepts lifespan messages, would be to send a handshake, eg. {"type": "lifespan.handshake", "asgi": {"version": 3}, "protocols": ["http", "websocket", "lifespan"]}.

Simple client implementations can just do:

async def asgi(scope, receive, send):
    assert scope["type"] == "http"
    ...

Which would be enough for a server to determine that "no I didn't get a handshake, I got an exception. I'll turn lifespan off", rather than "I started the lifespan, sent 'startup', and got an exception, I don't know if that's because the application doesn't support lifespan, or it does but failed to complete application startup".

Summary:

  • Yeah, a version bump, and servers that support both styles would make sense.
  • An ASGI handshake message on the lifespan protocol would help deal with an existing issue of "can't really determine which message types the client supports', that gets more problematic in the v3 style. (Since there's no "raise an exception on init", before the main call is started anymore)

Making those two changes together looks like it'd make sense to me.

@andrewgodwin

This comment has been minimized.

Copy link
Member

andrewgodwin commented Feb 23, 2019

OK, I think we agree on the version bump then (to 3). I can pop back in to Daphne and get that patch written once the spec is updated, but I want to solve the other thing first.

My design assumption with things like lifespan was that the app would just ignore protocols it didn't support, rather than raising exceptions. I'm not sure if I want to make the app communicate back to the server when it starts, since that starts to defeat the whole keep-it-relatively-simple aspect.

@tomchristie

This comment has been minimized.

Copy link
Member Author

tomchristie commented Feb 25, 2019

My design assumption with things like lifespan was that the app would just ignore protocols it didn't support, rather than raising exceptions. I'm not sure if I want to make the app communicate back to the server when it starts, since that starts to defeat the whole keep-it-relatively-simple aspect.

Indeed yeah.

Question being how (or if) we best communicate which protocols (and version) the client supports.

E.g. if we just ignored lifespan messages, then servers that supported it would (by default) send a startup, and just hang waiting or timeout with a "failed to startup message".

It could be that the onus is on developers to enable to disable the server protocols.

Tho it's also a bit awkward to have to tell devs that the right way to run a basic "hello, world" ASGI app with, say, uvicorn, is actually:

$ uvicorn example:App --lifespan none --ws none

I guess if we really want a robust ability to upgrade the protocol over a long period of time then we really will want a both-way server & client exchange of capabilities, but I also agree with the general keep-it-relatively-simple motivation.

@almarklein

This comment has been minimized.

Copy link
Contributor

almarklein commented Feb 25, 2019

Question being how (or if) we best communicate which protocols (and version) the client supports.

Can't we simply still use exceptions for this? From what I understand the major problem is that it's hard to discern an "I can't handle this protocol" from "whoops, I failed trying". What about specifying a specific exception type? NotImplementedError seems to fit nicely :) I guess applications that are more than a toy example should make sure to catch NotImplementedError to avoid it from falling through. Or, we can require the exception to have a certain attribute set.

@tomchristie

This comment has been minimized.

Copy link
Member Author

tomchristie commented Feb 25, 2019

Can't we simply still use exceptions for this?

Yeah, we can. The only problem is lifespan - if the server starts the callable and sends a "started", then once it handles the exception there's no way to differentiate between "startup failed" - in which case the server should just stop. (Eg. couldn't connect to the database). and "lifespan not supported".

If the first thing that the lifespan protocol does is send back a "hi there" message, then you can differentiate between the two cases.

The NotImplementedError idea might be okay. Perhaps there's a risk that if your codebase erronously raises it elsewhere then it could end up silently turn off lifespan or websocket handling.

@almarklein

This comment has been minimized.

Copy link
Contributor

almarklein commented Feb 25, 2019

Perhaps there's a risk that if your codebase erronously raises it elsewhere then it could end up silently turn off lifespan or websocket handling.

Maybe something like this?

In the application:

    ...
    else:
        raise NotImplementedError("asgi-" + scope["type"])

In the server:

try:
    ...
except NotImplementedError as err:
      if err.args and err.args[0] == "asgi-lifespan":
             ...  turn lifespan off
       else:
            raise
@andrewgodwin

This comment has been minimized.

Copy link
Member

andrewgodwin commented Mar 1, 2019

Yes, lifespan really does gum up the works because it's the only protocol the server actually waits on; other protocols aren't going to have the issue where if you don't support them your entire app freezes.

I'm not super keen on re-using NotImplementedError to mean "ignore this", but I don't see many ways out of this without annotating applications saying "hey I support lifespan!". I wouldn't be against that either, but I'd want the default to be no-lifespan and then you have to explicitly turn it on.

@tomchristie

This comment has been minimized.

Copy link
Member Author

tomchristie commented Mar 1, 2019

Yes, lifespan really does gum up the works because it's the only protocol the server actually waits on; other protocols aren't going to have the issue where if you don't support them your entire app freezes.

Yeah. I'm reasonably happy with the "send a 👍 as the very first thing the lifespan protocol does" because it's at least limited to lifespan. It doesn't even necessarily need include any handshake info such as a list of supported protocols, it could just be a "I'm ready to startup when requested". It solves the issue of "did lifespan raise an exception because startup failed, or just because it wasn't supported?".

The rule of thumb for devs to follow wrt. supporting protocols can simply be:

  • Applications/Endpoints should strictly check the protocol is a supported type, and error otherwise.
  • Middleware should pass unknown protocol types through unmodified.

So, eg, this would be enough to gracefully disable lifespan and websockets:

async def hello_world(scope, receive, send):
    assert scope['type'] == 'http'
@tomchristie

This comment has been minimized.

Copy link
Member Author

tomchristie commented Mar 1, 2019

An alternative: Have an explicit “startup failed” message for the lifespan protocol, which indicates to the server that it should abort. That’s then properly distinct from the lazy exception case.

@tomchristie

This comment has been minimized.

Copy link
Member Author

tomchristie commented Mar 4, 2019

Summary of what I now think would be sufficient for a 3.0 release:

  • Single callable async def app(scope, receive, send), with backwards support for the double callable style.
  • Supplement the existing lifespan.startup.complete and lifespan.shutdown.complete messages with complementary lifespan.startup.failed and lifespan.shutdown.failed {"type": ..., "exc": ...} messages.

That'd enough that we don't neccessarily need to include any handshaking or supported-protocol info beyond the currently existing spec.

@tomchristie

This comment has been minimized.

Copy link
Member Author

tomchristie commented Mar 4, 2019

We don't strictly need the shutdown failed, but it'd seem to make sense for completeness.

@andrewgodwin

This comment has been minimized.

Copy link
Member

andrewgodwin commented Mar 4, 2019

How does "startup failed" help differentiate the non-lifespan-enabled app and the lifespan-enabled app that wishes to exit, though? A non-lifespan enabled app will never send complete or failed and so the server will sit there forever.

@tomchristie

This comment has been minimized.

Copy link
Member Author

tomchristie commented Mar 4, 2019

It's sufficient so long as apps check they're getting supported protocols, and raise errors (of any kind) if they don't.

@andrewgodwin

This comment has been minimized.

Copy link
Member

andrewgodwin commented Mar 4, 2019

Ah, so we say that "if you get an error back from startup you continue" whereas "if you get a failed back from startup you stop"? I can go with that.

@andrewgodwin

This comment has been minimized.

Copy link
Member

andrewgodwin commented Mar 4, 2019

I have made these changes, including a backwards-compatability module in asgiref, in this pull request: #80

Comments more than welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.