miguelgrinberg / flask-sock

Modern WebSocket support for Flask.
MIT License
272 stars 24 forks source link

Compatibility with `WsgiToAsgi` middleware #58

Closed Archmonger closed 1 year ago

Archmonger commented 1 year ago

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.

miguelgrinberg commented 1 year ago

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.

Archmonger commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

Archmonger commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.