narrow _monitor_process except to KeyError#770
Open
HrachShah wants to merge 1 commit into
Open
Conversation
The bare `except:` in ProcessManager._monitor_process was catching every exception and silently swallowing it. The only operations inside the try block are dict del/pop (which raise KeyError) and JsonRpc.close() which already wraps its operations in `contextlib.suppress(Exception)` and therefore cannot raise. Replacing the bare except with `except KeyError` makes the failure mode visible to a future maintainer who introduces a new call inside the try block that could raise an unrelated exception type.
HrachShah
force-pushed
the
fix/jsonrpc-monitor-narrow-exception
branch
from
54f288c to
d4ec4fa
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
{"body": "## Summary\n\nReplaces the bare
except:clause inProcessManager._monitor_process(the background thread that waits for a workspace's tool subprocess to exit and then cleans up the registry entries) with aKeyError-only handler.\n\n## Rationale\n\nThe only operations in thetryblock are:\n\n-del self._processes[workspace]— raisesKeyErrorif the workspace key has already been removed bystop_processor another monitor\n-self._rpc.pop(workspace)— raisesKeyErrorfor the same reason\n-rpc.close()— internally wraps every close call incontextlib.suppress(Exception), so it cannot raise\n\nNothing in the block can raise aBaseExceptionsubclass that we need to silence other thanKeyError. The original bareexcept:was atry/except-with-no-purpose: aSystemExitorKeyboardInterruptraised here (extremely unlikely from adel/pop/closecall, but conceptually possible if some CPython implementation changed) would have been silently swallowed, which is the wrong default for a thread-pool callback.\n\n## Test plan\n\n-python -m py_compile bundled/tool/lsp_jsonrpc.pypasses\n- Lint:pylint bundled/tool/lsp_jsonrpc.pyshows no new warnings\n- Manually traceable: each line in thetryblock has a documented exception type in the Python data model, andKeyErrorcovers all of them\n"}