Enhance error handling in readLoop #2409
Conversation
…type assertion for request cancellation
There was a problem hiding this comment.
Pull request overview
This PR enhances error handling in the LSP server's readLoop by adding panic recovery at the goroutine level and converting an unsafe type assertion to a safe checked assertion for cancel request parameters.
Key changes:
- Adds a deferred panic recovery handler around the
readLoopgoroutine execution to prevent crashes from propagating to the main process - Replaces the unsafe type assertion for
CancelParamswith a safe checked assertion to avoid panics from type conversion failures
| if v, ok := req.Params.(*lsproto.CancelParams); ok { | ||
| s.cancelRequest(v.Id) | ||
| } |
There was a problem hiding this comment.
Silently ignoring a failed type assertion for CancelParams could hide protocol violations or bugs in the message parsing logic. If req.Params is not of type *lsproto.CancelParams when Method is MethodCancelRequest, this represents a serious protocol error that should be logged or handled explicitly rather than silently ignored. Consider logging a warning or error when the type assertion fails.
| go func() { | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| stack := debug.Stack() | ||
| s.logger.Errorf("panic in readLoop: %v\n%s", r, string(stack)) | ||
| readLoopErr <- fmt.Errorf("panic in readLoop: %v", r) | ||
| } | ||
| }() | ||
| readLoopErr <- s.readLoop(ctx) | ||
| }() |
There was a problem hiding this comment.
The panic recovery mechanism conflicts with the project's coding guidelines which state "Do not remove any debug assertions or panic calls. Existing assertions are never too strict or incorrect." This guideline suggests that panics are intentional indicators of programming errors that should be caught during development. If readLoop contains panics, they likely indicate bugs that should be fixed rather than recovered from. Consider whether masking panics with recovery is the appropriate solution, or whether the underlying causes of potential panics should be addressed instead.
|
Why add this? When does |
2 participants
Add recover at goroutine call sites to prevent panics inside goroutines from affecting the main process; also add assertion checks for type conversions to avoid panics caused by failed assertions