Contain backend OSErrors at the command-execution boundary - #2
Open
ninowalker wants to merge 1 commit into
Open
Conversation
A custom filesystem backend that raises anything outside the three errors the commands recognize propagates out of bash.exec() as a Python exception instead of a nonzero exit. Network-backed backends have no exception to fail through at all. Add one except OSError clause to the try/finally in _execute_simple_command, which spans builtin dispatch, command dispatch and output redirection. It reports "<cmd>: <filename>: <strerror>" and exit 1, and only sees errors no command handled, so existing messages are unchanged. OSError only: the interpreter's control-flow exceptions derive from InterpreterError and cannot be swallowed, and a non-OSError means the backend is broken and should crash rather than become exit 1. Document the expected exception taxonomy for backend authors in the README.
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
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.
Implements option (a) from dbreunig#9, with a little of option (b) alongside it.
The problem
Commands catch a narrow, specific set of filesystem errors —
catcatches exactlyFileNotFoundError,IsADirectoryErrorandPermissionError— so a custom backend that raises anything else propagates out ofbash.exec()as a Python exception instead of becoming a nonzero exit with a message on stderr. A network-backed filesystem has no exception to fail through: a timeout, a connection reset or a plainOSErrorall escape. That breaks the containment property the sandbox exists to provide, and pushes atry/exceptaround everyexec()call into every harness. The redirect path already handles this correctly (redirection failures are wrapped and become exit 1), so the command path is the inconsistency.The design
One catch at the command-execution boundary, not per-command.
_execute_simple_commandalready has atry/finallyspanning command-name expansion, builtin dispatch, command dispatch and output redirection — the point where a simple command'sExecResultis produced. Adding anexcept OSErrorclause to it is a single handler covering all of them, including builtins liketest -Lthat reach the filesystem without going through a command object. The existing per-command handlers are untouched and still run first, socat: /x: No such file or directoryis still phrased bycat; the backstop only ever sees what no command claimed.OSErroronly, deliberately — this is notexcept Exception. The line is between the environment failing and the code being wrong:OSErroris the environment failing. A shell contains that and reports it as a nonzero exit.TimeoutErrorandConnectionErrorareOSErrorsubclasses, so remote and network-backed filesystems arrive here for free — no new taxonomy to invent.AttributeErrororTypeErrorintoexit 1would turn a broken backend into a shell that quietly misbehaves, which is strictly worse than a traceback.The interpreter's own control flow —
ExitError,ReturnError,BreakError,ContinueError,ErrexitError— derives fromInterpreterError(Exception), notOSError, so the backstop structurally cannot eat it. Tests pin that.Message shape is
<cmd>: <filename>: <strerror>when the error carries a filename,<cmd>: <strerror or str(e)>when it does not, on stderr with exit code 1. Because it is an ordinary nonzeroExecResult, it composes with everything already built on exit codes: pipelines take the failing segment's code,set -estops,||and&&branch normally.The tests
tests/test_interpreter/test_fs_error_boundary.pyuses aFlakyFswrapper that delegates toInMemoryFsand raises a chosen error on chosen paths. Ten cases in three groups:OSErrorwith an errno and filename, and a bareTimeoutError, each become exit 1 with the expected stderr and do not raise; the shell keeps running afterwards;set -estops on it. These four fail without the change (the exception escapesbash.exec()).ENOENT,EISDIRandEACCESstill producecat's own coreutils-accurate messages, not the generic one.exit 3still yields 3,breakstill leaves the loop,return 4still sets$?.The six tests in the last two groups pass with and without the change, which is the point: the backstop adds a path and moves nothing that already worked.
Docs
One paragraph under a new Custom Filesystems heading in the README, next to the other filesystem docs where a backend author is already reading: raise
OSErrorsubclasses; the three recognized ones get command-specific messages; everything else is formatted generically by the boundary; non-OSErrorpropagates because it means something is broken.Scope
Input redirection for simple commands (
< file) is handled inline before thistryblock and keeps its existingFileNotFoundError/IsADirectoryErrorhandling — extending the boundary upward to cover it would mean re-indenting about sixty lines of deeply nested code, so I left it out rather than bury the change in a reformat. Happy to do it separately if it's wanted.Testing
pytest tests/test_commands/ tests/test_network.py(the CI command): 2314 passed, 1 skippedtests/spec_testsandtests/comparison_tests: 2923 passed, 3 skippedtests/spec_tests+tests/comparison_tests: identical results tomain(pre-existing failures, unchanged count)Note
Staged here on the fork rather than sent upstream, pending the design conversation on dbreunig#9. Intentionally no
Fixesline so this closes nothing.