Detect if we received EOF due to a command that failed #551
Conversation
apyrgio
force-pushed
the
2023-09-check-exit-codes
branch
from
9b61457
to
65c8540
Compare
apyrgio
changed the title
apyrgio
force-pushed
the
2023-09-check-exit-codes
branch
2 times, most recently
from
72a88ac
to
eeb3760
Compare
|
@deeplow I checked this PR against your PR for error handling, and it seems to work ok, so I promoted it as ready for review. Feel free to give it a look. |
There was a problem hiding this comment.
Only added a note on the fact that we have now two args and one (p.stdout) can be accessed from the other (p).
Other than that it looks good. The only extra thing is that this essentially removes the need for the InterruptedConversion exception from #546 as this will catch the exit code before it detects that it received less data than it expected. But I think that's OK.
|
This also has issues with the It basically becomes a race condition when the test manages to pipe the input file into the process before it exits diff --git a/tests/isolation_provider/test_qubes.py b/tests/isolation_provider/test_qubes.py
index 70d57463..b3f1ddad 100644
--- a/tests/isolation_provider/test_qubes.py
+++ b/tests/isolation_provider/test_qubes.py
@@ -79,6 +79,7 @@ class TestQubes(TestIsolationProvider):
stderr=subprocess.PIPE,
shell=True,
)
+ time.sleep(0.2)
return p
Otherwise it will fail when trying to read |
|
Regarding your Regarding the |
apyrgio
force-pushed
the
2023-09-check-exit-codes
branch
from
eeb3760
to
faa18b5
Compare
|
I have reworked the PR to catch errors caused by EOF in a separate place. I think this should address your concerns about our helpers and the |
Store, in an instance attribute, the process that we have started for the spawned disposable qube. In subsequent commits, we will use it from other places as well, aside from the `_convert` method. Note that this commit does not alter the conversion logic, and only does the following: 1. Renames `p.` to `self.proc.` 2. Adds an `__init__` method to the Qubes isolation provider, and initializes the `self.proc` attribute to `None`. 3. Adds an assert that `self.proc` is not `None` after it's spawned, to placate Mypy.
Add an error for interrupted conversions, in order to better differentiate this scenario from other ValueErrors that may be raised throughout the code's lifetime.
If a conversion has been interrupted (usually due to an EOF), figure out why this happened by checking the exit code of the spawned process.
apyrgio
force-pushed
the
2023-09-check-exit-codes
branch
from
faa18b5
to
18b73d9
Compare
| @@ -41,7 +41,7 @@ def read_bytes(f: IO[bytes], size: int, timeout: float, exact: bool = True) -> b | |||
| """Read bytes from a file-like object.""" | |||
| buf = nonblocking_read(f, size, timeout) | |||
| if exact and len(buf) != size: | |||
| raise ValueError("Did not receive exact number of bytes") | |||
| raise errors.InterruptedConversion | |||
There was a problem hiding this comment.
I missed this here, but I'll catch it in #546.
| raise errors.InterruptedConversion | |
| raise errors.InterruptedConversion() |
Handle incomplete reads due to EOF by checking if the underlying command has exited. If so, raise the corresponding exception.
Refs #430