get rid of some panics #1526
Merged
get rid of some panics #1526
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Remove an allocation in favour of deferring a call to release the response.
Return an error instead of panicking if the user supplied a nonsensical DialFunc.
If a compression level exceeding gzip's boundaries is provided, fasthttp will panic. Instead it would be better to handle this error for them by limiting it to the minimum or maximum value, depending on the direction the user has exceeded the limits. Clamp the value of gzip to always be between gzip.BestSpeed and gzip.BestCompression.
erikdubbelboer
requested changes
Mar 26, 2023
|
On Sun Mar 26, 2023 at 10:28 PM CEST, Erik Dubbelboer wrote:
Can you fix the linting errors.
Will do, still not completely through, I also want to add commentary
explaining why the remaining panics should stay.
--
Moritz Poldrack
https://moritz.sh
|
mpldr
requested review from
erikdubbelboer
and removed request for
erikdubbelboer
When a negative count is reached when unregistering a connection, a panic is caused even though data-integrity is not at risk. Replace the panic() with a simple clamp on the value to ensure the value does not exceed it's expected lower bounds. References: #1504
Since there is no way of handling or even logging non-critical errors in stateless non-blocking writecalls, just drop them and hope the user notices and tries again.
Instead of panicking for invalid behaviour, it's preferable to just turn the function into a noop.
Since bufio already panics on negative reads, it is not necessary to do so as well. If the length is zero and for some reason no error is returned, readBodyIdentity and appendBodyFixedSize now errors in these cases. Link: https://github.com/golang/go/blob/851f6fd61425c810959c7ab51e6dc86f8a63c970/src/bufio/bufio.go#L246
When a negative count is reached when unregistering a reader, a panic is thrown even though data-integrity is not at risk. Replace the panic() with a simple clamp on the value to ensure the value does not exceed it's expected lower bounds.
Panicking with "BUG: " obscures the error. As the segfault causes a panic anyway, just let the chaos unfold.
Writing on a timed-out response is not endangering data integrity and just fails.
|
/shrug Can't do anything about the tests taking too long |
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses #1522 and #1504. The latter is not fixed, but only referenced as the underlying double-close is not resolved.
Fixes: #1522