Skip to content

Fix flate reader reset#222

Open
rchernobelskiy wants to merge 1 commit into
gobwas:masterfrom
rchernobelskiy:patch-1
Open

Fix flate reader reset#222
rchernobelskiy wants to merge 1 commit into
gobwas:masterfrom
rchernobelskiy:patch-1

Conversation

@rchernobelskiy
Copy link
Copy Markdown

Thanks for the great lib!
Here is a small fix

Comment thread wsflate/reader.go
// ReadResetter is an optional interface that Decompressor can implement.
type ReadResetter interface {
Reset(io.Reader)
Reset(io.Reader, []byte) error
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks like this change isn't back compatible. Do we need to change public API?

@cristaloleg
Copy link
Copy Markdown
Collaborator

Can you explain what this fixes and how?

@rchernobelskiy
Copy link
Copy Markdown
Author

rchernobelskiy commented May 17, 2026

@cristaloleg Sorry I should have explained it in the PR.
When using wsflate.Reader, we need to Reset it for every message, right?
So the wsflate.Reset function tries to reuse the existing flate Reader, instead of calling the passed-in callback func to create a new one for each message.
But the reuse path is never taken, because the flate Reader Reset func has a different signature:
https://pkg.go.dev/compress/flate#Resetter
With the submitted fix, the type assesrtion in wsflate.Reset succeeds, and Reset() is properly invoked on the previously created flate.Reader (if present), allowing proper reuse.

Why do you think that it's not backwards compatible and requires public API change? It doesn't seem like it to me from first glance. (Edit: oh you mean change to the ReadResetter interface? Yes, I suppose there is not an easy way to get around that other than perhaps creating a new one/a new path)
One can of course work around this issue by wrapping the flate Reader in a custom type, but I think that's not the intended approach.

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.

2 participants