Repository navigation
Size the relay test's payload from the pipe capacity - #1801
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1801 +/- ##
=======================================
Coverage 99.75% 99.75%
=======================================
Files 23 23
Lines 6593 6593
=======================================
Hits 6577 6577
Misses 16 16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
* Size the relay test's payload from the pipe capacity. See also: python-cmd2/cmd2#1801
tleonhardt
left a comment
tleonhardt
left a comment
There was a problem hiding this comment.
@setarcos
Thanks for tracking this down. Sizing the payload from F_GETPIPE_SZ is the right fix, and it stops the lend assertion from failing on large-page kernels. One side effect: on Linux, the test no longer covers the ordering case at its end ("cmd2's own writes follow what the child wrote, which is still pending").
With first_read = len(payload) - capacity, exactly one pipe's worth is left after the pause, so the relay can move all of it into the consumer's pipe before writer.write(b"end"). flush() then has nothing to wait for. I wrapped _DescriptorRelay.flush() to record how many bytes the relay still held when it was called (pauses=True, 5 runs each, same result every run):
| Platform | This PR | With the suggestion below |
|---|---|---|
| macOS (arm64) | 49152 | 65536 |
| Linux amd64, 4 KiB pages | 0 | 16384 |
| Linux arm64, 4 KiB pages | 0 | 16384 |
macOS only looks fine because the relay writes in PIPE_BUF (512-byte) chunks, which keep the consumer's pipe at 16 KiB rather than 64 KiB. With the old 280000/200000 sizes, about 14 KiB stayed in the relay on 64 KiB pipes, and the suggestion below brings that back. The test passes with it on all three platforms, but I have no 16 KiB- or 64 KiB-page kernel to try it on. Could you confirm it passes on yours?
Two other relay tests make the same 64 KiB assumption. They're outside this diff and won't fail on large-page kernels, but there they pass without testing anything:
test_pipeline_writer_relay_lends_only_to_a_consumer_waiting_for_the_terminal(262144 bytes) expects the relay's pipe to fill up. With 1 MiB pipes, the consumer's pipe alone holds the payload.test_pipeline_writer_relay_leaves_the_terminal_after_a_producer_finishes(70000 bytes) is meant to be "more than the consumer's pipe holds". With 256 KiB pipes it isn't.
A small module-level helper for the capacity lookup, used by all three tests, would cover them: e.g. capacity + 4096 and 4 * capacity, which roughly match today's sizes on 64 KiB pipes. If you scale the 262144 one, scale its os.read(read_fd, 16384) too (say capacity // 4). Otherwise, with 1 MiB pipes, the test makes 256 reads × 0.15 s and runs past its 30 s timeout. I'm happy to take that as a follow-up if you'd rather keep this PR small.
Minor and optional: the 2 * 65536 in the payload stands in for the relay's
os.read(self._in_fd, 65536) in cmd2/utils.py. A shared constant would keep the two from drifting apart.
test_pipeline_writer_relay_lends_only_while_a_producer_waits hardcoded a 280000-byte payload and a 200000-byte first read, assuming the 64 KiB pipes of a 4 KiB-page system. A pipe holds a fixed number of pages, so a 16 KiB-page kernel has 256 KiB pipes and a 64 KiB-page one 1 MiB pipes. There the relay drains the whole payload into the consumer's pipe before the child has to wait, no lend happens, and the test asserts a lend that was never needed. cmd2 itself behaves correctly; only the test's assumption depends on the page size. Size the payload from the consumer pipe's capacity, reported by F_GETPIPE_SZ where the kernel has it, and fall back to the 64 KiB that macOS and the BSDs cap a pipe at elsewhere. The consumer's first read leaves more than a pipe's worth pending, so part of the payload is still in the relay when cmd2 writes after it, and the rest fits in the pipes, so the child can finish.
4a0e4df to
1556e36
Compare
|
I ran it on Linux loongarch64(16 KiB pages) and it passes there too. |
test_pipeline_writer_relay_lends_only_while_a_producer_waits hardcoded a 280000-byte payload and a 200000-byte first read, assuming the 64 KiB pipes of a 4 KiB-page system. A pipe holds a fixed number of pages, so a 16 KiB-page kernel has 256 KiB pipes and a 64 KiB-page one 1 MiB pipes. There the relay drains the whole payload into the consumer's pipe before the child has to wait, no lend happens, and the test asserts a lend that was never needed. cmd2 itself behaves correctly; only the test's assumption depends on the page size.
Size the payload from the consumer pipe's capacity, reported by F_GETPIPE_SZ where the kernel has it, and fall back to the 64 KiB that macOS and the BSDs cap a pipe at elsewhere. The consumer's first read leaves more than a pipe's worth pending, so part of the payload is still in the relay when cmd2 writes after it, and the rest fits in the pipes, so the child can finish.