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

Expose ASGI scope on Request object #2417

Closed
rmorshea opened this issue Mar 26, 2022 · 11 comments · Fixed by #2432
Closed

Expose ASGI scope on Request object #2417

rmorshea opened this issue Mar 26, 2022 · 11 comments · Fixed by #2432

Comments

@rmorshea
Copy link

rmorshea commented Mar 26, 2022

Is your feature request related to a problem? Please describe.
I'm trying to write an application framework that works across ASGI compatible server implementations and to do this I'd like to make the ASGI scope available to users. Sanic does not make the scope publicly available though.

Describe the solution you'd like
I'd like the sanic.request.Request object to provide a scope attribute which is the current ASGIScope

Additional context
Presently I'm working around this by accessing request.app._asgi_app.transport.scope

@ahopkins
Copy link
Member

That's a good idea, and sounds easy enough that we can probably squeeze that in for the upcoming release.

@rmorshea
Copy link
Author

rmorshea commented Mar 26, 2022

You'll probably need to handle the case where the server isn't being run via ASGI and thus there is no _asgi_app with some informative error message, but from what I can tell it seems like the project is moving away from running without ASGI.

@ahopkins
Copy link
Member

from what I can tell it seems like the project is moving away from running without ASGI.

Just curious, what makes you think this is the case? The built-in Sanic server (TMK) is still the most common deployment method. But, I do agree that ASGI support is an important alternative.

@rmorshea
Copy link
Author

rmorshea commented Mar 27, 2022

I can't recall exactly, but I may have skimmed this thread and came away with the wrong idea.

@ahopkins
Copy link
Member

No, not necessarily. ASGI is very important and I hope will continue to be. But we are not getting rid of the internal server either. Being geared specifically for Sanic it has the benefit of being optimized for performance in ways that ASGI servers cannot be.

@azimovMichael
Copy link
Contributor

Hi, I would like to take this issue, as my first contribution to the project.
So from the little I understand in the codebase, I thought about doing it like this:

  1. Adding an Optional scope argument to the Request object that will default None.
  2. When creating an ASGIApp instance, pass the scope param to the request_class variable.
    WDYT? @ahopkins @rmorshea

@rmorshea
Copy link
Author

That sounds reasonable, but then again, you could also make it a @property and return None if there's no _asgi_app.

I'm not very familiar with Sanic's internals though, so I can't say what the best option would be.

@azimovMichael
Copy link
Contributor

azimovMichael commented Apr 14, 2022

Yeah, I can do that, but I guess the downside is that currently the Request object is decoupled completely from any ASGI internals, and this change will change that a bit.
I'll wait for @ahopkins to weigh in

@ahopkins
Copy link
Member

@azimovMichael I would be happy for you to take this on. I see you have been assigned it already. Sorry for my delay, I am just now getting back to my regular schedule after some time off.

Instead of altering the __init__, we could just assign it:

instance.request = request_class(
    ...
)
instance.request.stream = instance
...
instance.request._scope = scope

Using @property as suggested by @rmorshea is a nice idea so that it is less easily changed and a pattern used elsewhere on the Request object.

@ahopkins
Copy link
Member

I would go so far as to do something like this:

@property
def scope(self):
    if not self._scope:
        raise NotImplementedError("...")
    return self._scope

@ahopkins
Copy link
Member

Actually... as I think about it more, I do not think that we even need to set it on the Request object at all. You should have access to it (I believe):

request.transport.scope

Therefore, it is just a matter of returning that in the property and making sure that you are in ASGI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants