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

[bug] Open websockets shouldn't count towards connection throttling limit #1314

Closed
Sentynel opened this issue Jan 6, 2023 · 0 comments · Fixed by #1315
Closed

[bug] Open websockets shouldn't count towards connection throttling limit #1314

Sentynel opened this issue Jan 6, 2023 · 0 comments · Fixed by #1315
Labels
bug Something isn't working

Comments

@Sentynel
Copy link
Contributor

Sentynel commented Jan 6, 2023

Describe the bug with a clear and concise description of what the bug is.

Just gonna copy kim's comments from Matrix in here:

i just took a look, and it basically does net/http hijack on the incoming UPGRADE request connection
soooo, there might actually be an easy way of handling this
we're currently holding open the main request handler with the streaming loop
but if the net/http hijack logic allows us to return that handler and perform the stream loop in another goroutine, that would mean throttling won't be an issue with websockets :)
in fact if that is the case, then that would be advantageous as it still allows us to throttle the streaming UPGRADE endpoint instead of completely ignoring it
by the looks of things it should be fine to return from the upgrade handler: https://pkg.go.dev/net/http#Hijacker

basiccally all we should need to do is

1234567891011go func() {
    // create new websocket context as the original net/http
    // request context is only valid until the calling handler returns.
    ctx, cncl := context.WithCancel(context.Background())
    defer cncl()

    for {
        // do some shit with wsConn, yada yada ...
    }
}()

and the logic from here onwards can go in that goroutine:

What's your GoToSocial Version?

git

GoToSocial Arch

No response

Browser version

No response

What happened?

No response

What you expected to happen?

No response

How to reproduce it?

No response

Anything else we need to know?

No response

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant