-
Notifications
You must be signed in to change notification settings - Fork 324
Support client_max_window_bits and server_max_window_bits compression options #534
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
base: master
Are you sure you want to change the base?
Conversation
Hey guys these ☝️ are failing due to thirdparty dependency. @nhooyr the third party dependency I introduce here is a well known drop in replacement for compress/flate. It also has constructors that allows you to specify the size of the window. |
Hey @hexdigest. Thanks for the contribution! I'll need to read up on this a bit to give a proper review, so it might take a while. In the meantime, I'm thinking an implementation that allows providing a custom flate implementation via options would be preferable. This way we don't need to add any external dependencies to this library, yet we support the use-case. It'd be fine to even include an example of how to do it as part of the go docs. |
@hexdigest is this something you still want to work on? |
Yep, I can do it. My only concern about the options approach is that depending on the provided flate implementation we should enable or disable certain compression options. But I guess this could be a part of flate abstraction interface (that can return a list of supported options) and manage pools. Does it make sense to you? |
What kind of use-cases are you considering? Off the top of my head I could see us having something like this: // Match stdlib flate.Writer, add release.
type FlateWriter interface {
io.WriteCloser
Flush() error
Reset(io.Writer)
Release() // Allow backing implementation to restore to pool.
}
type Options struct {
FlateWriter func(w io.Writer, bits int) FlateWriter
} But open to other suggestions, especially if this doesn't cover some use-case. |
I was thinking about something like this ☝️ maybe with the one minor difference which is putting Release() logic into Close() method of WriteCloser. But the thing that I was talking about in the above message is that depending on the FlateWriter implementation the websocket implementation (e.g. *_max_window_bits) might or might not accept certain compression options passed by the client. So maybe we can add a method to this FlateWriter interface that returns compression options that it supports. WDYT? |
Yeah, that's honestly better 👍🏻
That could overcomplicate the interface. If I'm understanding you correctly, I think we could cover the use-case by adding Usage would then look something like: c, err := websocket.Accept(w, r, &websocket.AcceptOptions{
CompressionServerWindowMinBits: 9,
CompressionServerWindowMaxBits: 15,
CompressionFlateWriter: kpFlateWriter(kpflate.DefaultCompression),
})
func kpFlateWriter(level int) func(io.Writer, int) FlateWriter {
return func(dst io.Writer, bits int) FlateWriter {
// ...
}
} Does that make sense? Edit: Perhaps best to add |
This is related and fixes #443