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

Compatibility with WsgiToAsgi middleware #58

Closed
Archmonger opened this issue Jun 15, 2023 · 8 comments
Closed

Compatibility with WsgiToAsgi middleware #58

Archmonger opened this issue Jun 15, 2023 · 8 comments
Labels
question Further information is requested

Comments

@Archmonger
Copy link

Archmonger commented Jun 15, 2023

asgiref.sync.WsgiToAsgi does not work with flask-sock due to several factors

The main issue appears to be that WsgiToAsgi assumes that WSGI apps are only capable of scope["type"] == "http".

I attempted to remove all the WsgiToAsgi restrictions but it resulted in both broken HTTP and Websockets. There might need to be some collaborative PRs between this repo and django/asgiref in order to get this working.

Sync Websockets are possible within ASGI as proven by django/channels. Which means that WsgiToAsgi middleware should, in theory, be able to support this.

@Archmonger Archmonger changed the title Compatibility with WsgiToAsgi middleware Compatibility with WsgiToAsgi middleware Jun 15, 2023
@miguelgrinberg
Copy link
Owner

miguelgrinberg commented Jun 15, 2023

The WSGI protocol does not support WebSocket. Flask-Sock implements a number of non-standard WSGI extensions used by Gunicorn, Werkzeug, Eventlet and Gevent that allow WebSocket to work in spite of WSGI not supporting it.

You will need to extend WsgiToAsgi so that the WSGI context that is passed to Flask and Flask-Sock uses one of the supported server extensions to WSGI. Like for example, you can add the gunicorn.socket entry to the environ dictionary with the actual socket with the client connection.

@miguelgrinberg miguelgrinberg added the question Further information is requested label Jun 15, 2023
@Archmonger
Copy link
Author

Archmonger commented Jun 15, 2023

Would it be reasonable (and/or technologically possible) for flask_sock.utils.WsgiToAsgi to exist within this repo that auto-configures compatibility with these non-standard WSGI extensions?

@miguelgrinberg
Copy link
Owner

I'm not sure, to be honest. I think this would make more sense to support (maybe optionally) on the asgiref project, but I'm not sure how they'll feel about it, given that all the ways to pass on a socket to a WSGI app are non-standard.

Without having context for your project, to me it feels strange that you are working with Flask and Flask-Sock but you are using an ASGI web server. Why not use FastAPI or Quart instead, which have native ASGI support for WS?

@Archmonger
Copy link
Author

ReactPy supports several different web frameworks: Flask, FastAPI, Sanic, Tornado, Django, Jupyter, Plotly-Dash. The long term goal is to support every major framework that has websocket support.

Running with an ASGI webserver isn't mandatory here, but it would be "nice to have".

Knowing the Django team, it's very unlikely they'd be willing to accept non-standard extensions within the asgiref repo.

@miguelgrinberg
Copy link
Owner

How about your project including a wrapper to WsgiToAsgi that adds the socket to environ, without replicating the original WsgiToAsgi functionality?

@Archmonger
Copy link
Author

Archmonger commented Jun 15, 2023

Definitely possible, but at that point it feels more appropriate to contribute that wrapper to this repo. I'm sure some other Flask enthusiasts would enjoy using that.

@miguelgrinberg
Copy link
Owner

To be honest, adding such feature here would give the wrong message. People may think that this is a supported and approved way to run this extension, while in fact it is a very bad idea, and may put additional support effort on my part in the future, if things were to break.

Let's not forget that what I'm describing here as a solution is a hack. You would be impersonating Gunicorn or any of the other web servers I support. I would be open to add support for the WsgiToAsgi class, if it exposed the socket using their own extension to WSGI. I already support four different web servers, it would be no problem to add one more, as long as the support burden is on their side.

@Archmonger
Copy link
Author

I'm realizing now that asgiref.wsgi.WsgiToAsgi doesn't even support file handles in POST requests,

This sounds like a bottomless rabbit hole, which isn't really worth it for a non-critical feature on our repo.

@Archmonger Archmonger closed this as not planned Won't fix, can't repro, duplicate, stale Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants