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

Doc inaccuracy related to handler option WithCompression #492

Closed
jhump opened this issue Apr 5, 2023 · 1 comment · Fixed by #493
Closed

Doc inaccuracy related to handler option WithCompression #492

jhump opened this issue Apr 5, 2023 · 1 comment · Fixed by #493
Labels
bug Something isn't working

Comments

@jhump
Copy link
Member

jhump commented Apr 5, 2023

The doc comment for WithCompression states:

Calling WithCompression with an empty name or nil constructors is a no-op.

However, tracing through the code, this is not true. The constructors are never checked for nil. Instead, any existing compressor previously registered with the same name is blindly overwritten with an invalid one. It is invalid because if either constructor is nil and ends up needing to be used, it will cause a nil dereference panic in the compressionPool here and here.

Perhaps this should instead behave like WithAcceptCompression? In that option, the constructors are checked. But if they are both nil, it effectively unregisters the named compression algorithm (instead of being a no-op).

@jhump jhump added the bug Something isn't working label Apr 5, 2023
@akshayjshah
Copy link
Member

Yep, definitely a bug. Suggested fix makes sense - I think we just need to return a nil from newCompressionPool in this case and add a test.

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.

2 participants