Strip root_path to simplify mounting as ASGI sub app #345

rodja · 2024-02-06T04:41:43Z

As an outcome of Kludex/starlette#2413 with newer Starlette versions we now have to provide the full path as engineio_path as done in the following example (via socketio.ASGIApp(socketio_server=sio, socketio_path='/ws/socket.io'):

# pip install "starlette>=0.33.0" python-socketio uvicorn
import socketio
from starlette.applications import Starlette
from starlette.responses import HTMLResponse
from starlette.routing import Route

async def homepage(_):
    return HTMLResponse("""
    <!DOCTYPE html>
    <html>
    <head>
        <title>SocketIO Test</title>
        <script src="https://cdn.socket.io/4.7.3/socket.io.min.js"></script>
        <script type="text/javascript">
            document.addEventListener('DOMContentLoaded', (event) => {
                var basePath = window.location.pathname;
                var socket = io({ path: basePath + "ws/socket.io" });
                socket.on('msg', function(msg) {
                    document.body.innerHTML = '<h1>' + msg + '</h1>';
                });
            });
        </script>
    </head>
    <body>
        <h1>Connecting to SocketIO...</h1>
    </body>
    </html>
    """)

sio = socketio.AsyncServer(async_mode='asgi', cors_allowed_origins='*')
sio_app = socketio.ASGIApp(socketio_server=sio, socketio_path='/ws/socket.io')
app = Starlette(routes=[Route("/", homepage)])
app.mount('/ws', sio_app)

@sio.on('connect')
async def handle_connect(sid, *args, **kwargs):
    await sio.emit('msg', 'Hello from Starlette!')

# Run the server with "uvicorn demo:app --reload" if your file is called demo.py

That works as long as the app is not itself mounted as a sub app like this:

sio = socketio.AsyncServer(async_mode='asgi', cors_allowed_origins='*')
sio_app = socketio.ASGIApp(socketio_server=sio, socketio_path='/ws/socket.io')
sub_app = Starlette(routes=[Route("/", homepage)])
sub_app.mount('/ws', sio_app)
app = Starlette()
app.mount('/sub', sub_app)

The above code fails because the frontend tries to connect to /sub/ws/socket.io but engineio simply checks if the scope['path'] starts with self.engineio_path.

This pull request fixes this by making python-engineio aware of ASGIs scope['root_path']. It strips the root path from scope['path'] before checking whether it matches self.engineio_path. This allows us to provide the relative socketio_path (which is then passed to engineio as engineio_path:

sio = socketio.AsyncServer(async_mode='asgi', cors_allowed_origins='*')
sio_app = socketio.ASGIApp(socketio_server=sio, socketio_path='socket.io')
sub_app = Starlette(routes=[Route("/", homepage)])
sub_app.mount('/ws', sio_app)
app = Starlette()
app.mount('/sub', sub_app)

The issue came up in zauberzeug/nicegui#2515 and zauberzeug/nicegui#2468. It should be fixed in python-enginio in my opinion to better support "ASGI sub app mounting" for everyone using python-engineio.

miguelgrinberg · 2024-02-06T09:51:41Z

I don't think this is the right change for this package.

The idea of socketio_path is that it is the same as path in the client. If your client has to use /sub/ws/socket.io, then this is what you should set as the Socket.IO path in the server. And then everything works, right? The fact that you are mounting apps on top of apps to build your server is not something the Socket.IO layer needs to care about.

The way I recommend that you set up your applications is to use my ASGIApp class as the dispatcher. With this model Socket.IO is a top-level app, and any requests that are not for Socket.IO can be forwarded to another ASGI app, such as your Starlette one. You don't have to use this model if you don't like it, but this is what I support.

rodja · 2024-02-07T03:33:12Z

Thanks for reviewing my PR and for your insights on the socketio_path and ASGIApp dispatcher setup.

I came across a discussion in the ASGI spec that elaborates on root_path's role, indicating internal handling for URL routing is expected from ASGI servers. My intention with this PR is to bring python-engineio in line with this aspect of the ASGI spec, aiming to enhance its adaptability and flexibility in modular setups like NiceGUI, where encapsulation and self-contained packaging are key.

I realize now that this adjustment could introduce a breaking change, for which I should have initially marked the PR as a draft and highlighted the potential impact more clearly. I'm eager to discuss the path towards ASGI spec compatibility and whether it's a direction you'd consider exploring together, ensuring we provide thorough documentation for a smooth transition.

Looking forward to your feedback on pursuing this alignment with the ASGI spec.

miguelgrinberg · 2024-02-07T10:03:52Z

As I said above, I don't see the current ASGIApp as being incompatible with the ASGI spec. I have designed this class as a top-level router that dispatches requests to the Socket.IO app, to an alternative ASGI app, or to a simple static file dispatcher. Routing to Socket.IO and static files is done on the full path. It is debatable if this is the best idea for static files, but for the Socket.IO path I especially want it to be the full path, so that it matches the path you configure in the client.

You should consider that the ASGIApp class is overkill in the way that you are using it. You are relying on Starlette to do the routing, so ASGIApp doing routing after Starlette makes no sense at all. For your use case, you need a much simpler ASGIApp-like class that does not do any routing and just sends all traffic to the Socket.IO server no questions asked.

Thinking over this, maybe the best option is to add an option that implements what I just said. For example, setting socketio_path=None disables any path checking and assumes all traffic is Socket.IO traffic. Then you can implementing your routing fully using Starlette.

rodja · 2024-02-08T03:28:12Z

Thinking over this, maybe the best option is to add an option that implements what I just said. For example, setting socketio_path=None disables any path checking and assumes all traffic is Socket.IO traffic. Then you can implementing your routing fully using Starlette.

That is a wonderful idea. I just altered this pull request accordingly. I thought its most consistent if the parameters static_files and other_asgi_app are ignored when engineio_path is set to none because they only make sense when using it as a top-level router.

An alternative could be to split the two variants into separate classes. For example ASGIRootApp and ASGISubApp. What do you think?

miguelgrinberg · 2024-02-09T18:56:32Z

Merged after fixing the remaining linter issues. Thank you so much!

strip root_path to simplify mounting as ASGI sub app

e6e8096

rodja marked this pull request as draft February 7, 2024 03:32

rodja mentioned this pull request Feb 7, 2024

Strip root_path to make ASGIServer from socketio compatible with mounting as sub-app zauberzeug/nicegui#2521

Merged

ASGIApp handles all traffic when engineio_path is set to None

f1e5174

add test for using ASGIApp as sub of another ASGI server

7c96a1a

rodja marked this pull request as ready for review February 9, 2024 05:29

miguelgrinberg closed this in 1dbd573 Feb 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Strip root_path to simplify mounting as ASGI sub app #345

Strip root_path to simplify mounting as ASGI sub app #345

Uh oh!

rodja commented Feb 6, 2024

Uh oh!

miguelgrinberg commented Feb 6, 2024 •

edited

Loading

Uh oh!

rodja commented Feb 7, 2024

Uh oh!

miguelgrinberg commented Feb 7, 2024 •

edited

Loading

Uh oh!

rodja commented Feb 8, 2024 •

edited

Loading

Uh oh!

miguelgrinberg commented Feb 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Strip root_path to simplify mounting as ASGI sub app #345

Strip root_path to simplify mounting as ASGI sub app #345

Uh oh!

Conversation

rodja commented Feb 6, 2024

Uh oh!

miguelgrinberg commented Feb 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rodja commented Feb 7, 2024

Uh oh!

miguelgrinberg commented Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rodja commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miguelgrinberg commented Feb 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

miguelgrinberg commented Feb 6, 2024 •

edited

Loading

miguelgrinberg commented Feb 7, 2024 •

edited

Loading

rodja commented Feb 8, 2024 •

edited

Loading