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

Fixes subprotocol selection (aling with rfc6455) #823

Merged
merged 6 commits into from Nov 7, 2023

Conversation

KSDaemon
Copy link
Contributor

Fixes #822

Summary of Changes

  1. Changed the order of subprotocol selection to prefer client one

@KSDaemon
Copy link
Contributor Author

Just received a notification from github with a comment from @FZambia.... But can not see it here....

@FZambia
Copy link
Contributor

FZambia commented Oct 28, 2022

@KSDaemon sorry, I deleted it - I missed the issue description in #822 originally and found what I asked about in its description.

@KSDaemon
Copy link
Contributor Author

O okay! Everything is clear now?

@FZambia
Copy link
Contributor

FZambia commented Oct 28, 2022

As for me I still think that the RFC is not very clear about exact precedence, but seems there are some implementations which prefer client order already. The change in this PR may break some code I suppose - so probably it should be optional/configurable. But I am not a maintainer of this lib - just following.

@KSDaemon
Copy link
Contributor Author

Well, I agree with you that it is not so clearly specified, but yeah, some other server implementations, that I know prefer the client's subprotocol in order of precedence.

And that makes sense. Because in other cases - the server will always use its first one, and if the client knows that the server supports a few, it has to send only one it is interested in most... a bit weird...

The tests are passing :) that is good. So I hope it won't break anything (much).

@KSDaemon
Copy link
Contributor Author

Any maintainers around here?
@garyburd Seems that you are the only maintainer (as looking into code committers). Would be great if you'll be able to respond somehow! Thnx!

@garyburd
Copy link
Contributor

I agree that subprotocol selection should use the client order.

Given the possibility that the PR breaks something, the PR will need to wait for a new maintainer.

@coreydaley
Copy link
Contributor

@KSDaemon I would like to see a unit test verifying that the order of the client protocols is preferred, then I think this would be good to be merged.

@KSDaemon
Copy link
Contributor Author

@coreydaley Sure! I'll have a look!

@pull-request-size pull-request-size bot added size/M and removed size/XS labels Sep 5, 2023
@KSDaemon
Copy link
Contributor Author

KSDaemon commented Sep 5, 2023

@coreydaley I've added tests. Please have a look when you'll have time!

@KSDaemon
Copy link
Contributor Author

@coreydaley gentle ping

@KSDaemon
Copy link
Contributor Author

Heh... yesterday was 1 year since I opened not the issue but PR with the fix! And it is still not merged.... And there is no indication that it will be merged in the foreseeable future....

@KSDaemon
Copy link
Contributor Author

KSDaemon commented Nov 6, 2023

@coreydaley gentle ping

@KSDaemon
Copy link
Contributor Author

KSDaemon commented Nov 7, 2023

Yahoo! @coreydaley let's merge then? I can not make it :)

@coreydaley coreydaley merged commit aa97606 into gorilla:main Nov 7, 2023
11 checks passed
@KSDaemon
Copy link
Contributor Author

KSDaemon commented Nov 7, 2023

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[bug] Websocket subprotocol is not chosen on client preferance
4 participants