narrow _monitor_process except to KeyError#770
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
|
🔒 Automated review in progress — @rchiodo is auto-reviewing this PR via the Review Center. Other automated reviewers defer to this claim to avoid duplicate, conflicting feedback. The claim is refreshed while the review is active; if it is not refreshed within the ownership timeout it is treated as vacant and another reviewer may take over. This is an automated marker. |
| rpc.close() | ||
| except: # pylint: disable=bare-except | ||
| except KeyError: | ||
| # KeyError is sufficient because the process is already dead |
There was a problem hiding this comment.
📍 bundled/tool/lsp_jsonrpc.py:185
The comment misstates the cause of the KeyError. The monitor only runs after proc.wait() returns, so the process is always dead at this point — that's not what triggers the KeyError. The KeyError arises because a concurrent path (e.g. stop_process or a restart's monitor) already removed the _processes/_rpc entries. Reword to e.g. # Another path (e.g. stop_process) may have already removed these entries.
|
🔒 Automated review in progress — @rchiodo is auto-reviewing this PR via the Review Center. Other automated reviewers defer to this claim to avoid duplicate, conflicting feedback. The claim is refreshed while the review is active; if it is not refreshed within the ownership timeout it is treated as vacant and another reviewer may take over. This is an automated marker. |
|
🔒 Automated review in progress — @rchiodo is auto-reviewing this PR via the Review Center. Other automated reviewers defer to this claim to avoid duplicate, conflicting feedback. The claim is refreshed while the review is active; if it is not refreshed within the ownership timeout it is treated as vacant and another reviewer may take over. This is an automated marker. |
|
🔒 Automated review in progress — @rchiodo is auto-reviewing this PR via the Review Center. Other automated reviewers defer to this claim to avoid duplicate, conflicting feedback. The claim is refreshed while the review is active; if it is not refreshed within the ownership timeout it is treated as vacant and another reviewer may take over. This is an automated marker. |
|
🔒 Automated review in progress — @rchiodo is auto-reviewing this PR. |
|
🔒 Automated review in progress — @rchiodo is auto-reviewing this PR. |
2 participants
{"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"}