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

Switch to fasthttp websocket #47

Closed
wants to merge 1 commit into from
Closed

Conversation

joeky888
Copy link

@joeky888 joeky888 commented May 5, 2023

Gorilla websocket is deprecated, let's use more maintained fashttp websocket implementation.

Signed-off-by: Joeky <joeky5888@gmail.com>
@CLAassistant
Copy link

CLAassistant commented May 5, 2023

CLA assistant check
All committers have signed the CLA.

@mstoykov
Copy link
Collaborator

Some observations about the library:

  1. it primarily exists to add websocket support to fasthttp server - we do not use that we use the client.
  2. the only changes related to the client seem to be some dependency bumps - we already do that
  3. There is a change from compress/flate to klauspost/compress which might yield some considerable improvements
  4. it still adds fasthttp as a new dependency.

I was going to request some data driven benchmarking on how one of them behaves better then the other but ... I don't see how they will be particularly different 🤷

I did run the autobahn test and it had exactly the same results so 🤷.

@olegbespalov do you have anything else to add.

Otherwise, my opinion currently is that if we are going to change from gorilla/websocket I would prefer to go to a library that actually changes stuff in a meaningful direction for us. Before that gorilla/websocket has been practically unmaintained for years. We have had some small problems but this fork does not address any of the ergonomic ones, unlikely to any of the performance ones, and definitely doesn't add any new functionality.

Copy link
Contributor

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@olegbespalov do you have anything else to add.

To be honest, you wrapped it. I'd say that we should investigate different libraries #9 and, in the scope of that task, decide to switch from gorilla to something that fits better.

@mstoykov
Copy link
Collaborator

Given that gorilla was unarchived I feel like there is no need for this PR at this point.

Thanks for bringing this up @joeky888 🙇

@mstoykov mstoykov closed this Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants