Closed blurfx closed 1 month ago
The recent changes enhance the errorToConnectError
function by implementing panic recovery, ensuring the server handles non-hashable errors without crashing. This addition improves error resilience while retaining the function's clarity and original logic.
Files | Change Summary |
---|---|
server/rpc/connecthelper/status.go | Added panic recovery in errorToConnectError function to prevent server crashes from non-hashable errors. Variables connectCode and ok were moved to explicit declarations for clarity. |
server/rpc/connecthelper/status_test.go | Introduced a test suite for errorToConnectError , validating error type conversion and ensuring correct behavior for converter.ErrPackRequired and mongo.CommandError . |
Objective | Addressed | Explanation |
---|---|---|
Server should not panic when handling non-hashable errors (#944) | ✅ |
🐇 "In the code where errors may lurk,
A safeguard now does its work!
With panics caught, we safely tread,
No more crashes, just stable threads.
A hop towards resilience, oh what a delight,
Errors handled, our future looks bright!" 🐰✨
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 51.03%. Comparing base (
54910c1
) to head (79e6402
).
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@krapie No, it doesn't. Should we print or log errors in this case?
@blurfx I think printing out error details may help us to take actions upon that specific error like mongo.CommandError
.
@krapie @blurfx It seems that an error will propagate to the callers and be output by the interceptor. 🤔
What this PR does / why we need it:
If an unhashable error is given, the error cannot be converted to a connecteError with a map, causing a panic and shutting down the server.
Which issue(s) this PR fixes:
Fixes #944
Special notes for your reviewer:
Would it be a good idea to log errors in recover?
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit