Skip to content

Size the remaining relay tests from the pipe capacity and share the relay chunk size #1803

Open

Description

@tleonhardt

Background

#1801 sized test_pipeline_writer_relay_lends_only_while_a_producer_waits from the consumer pipe's
capacity (F_GETPIPE_SZ on Linux, 64 KiB elsewhere), so it no longer assumes the 64 KiB pipes of a
4 KiB-page kernel. Two other relay tests in tests/test_utils.py still make that assumption. They
don't fail on 16 KiB-page (256 KiB pipes) or 64 KiB-page (1 MiB pipes) kernels. Instead they pass
without testing what they are meant to, so a regression in the relay's lending would go unnoticed
there.

Tests to fix

test_pipeline_writer_relay_lends_only_to_a_consumer_waiting_for_the_terminal uses a
262144-byte payload. It relies on the consumer's pipe filling up while a slow consumer drains it,
which brings the relay to the lend check in _DescriptorRelay._relay(). The relay must then decline
because no consumer is waiting for the terminal. With pipes of 256 KiB or more, the consumer's pipe
alone holds the payload, so the relay's writes never stall and assert not lends passes without
the check ever running.

test_pipeline_writer_relay_leaves_the_terminal_after_a_producer_finishes uses a 70000-byte
payload, meant to be "more than the consumer's pipe holds, but it fits in the relay". With pipes of
256 KiB or more, the consumer's pipe holds it, so the relay never waits on the consumer and
reader._lend_terminal.assert_not_called() passes without testing anything.

Shared relay chunk size

In the first test, payload = b"x" * (2 * capacity + 2 * 65536). The 2 * 65536 term covers the
chunk the relay holds in memory, os.read(self._in_fd, 65536) in _DescriptorRelay._relay()
(cmd2/utils.py), plus a margin of the same size. Nothing ties the two together. If the relay's
read size grows, the relay can absorb the whole margin, the child never blocks, and the test fails
on assert lent.is_set() after a 10 s stall with no hint why.

Give the chunk size a private module-level constant in cmd2/utils.py and use it in both places.
The 65536 fallback for the pipe capacity is a different quantity and should stay separate.

Suggested approach

  • Move the capacity lookup from Size the relay test's payload from the pipe capacity #1801 into a module-level helper in tests/test_utils.py, e.g.
    _pipe_capacity(fd). Import fcntl inside it, since the module also runs on Windows. Use the
    helper in all three relay tests.
  • ..._consumer_waiting_for_the_terminal: use a payload of 4 * capacity (262144 on 64 KiB pipes).
    Also scale the drain's os.read(read_fd, 16384) to capacity // 4, so the test stays at 16 reads
    × 0.15 s. Scaling only the payload means 256 reads (about 38 s) with 1 MiB pipes, past the test's
    30 s timeout.
  • ..._after_a_producer_finishes: use a payload of capacity + 4096 (69632 on 64 KiB pipes). That
    is more than the consumer's pipe holds, but fits in the relay's pipe plus its chunk.

Done when

  • The three relay tests size their payloads from the pipe capacity and the relay chunk constant,
    with no sizes hardcoded for 64 KiB pipes.
  • Each test still catches the behavior it guards on a 16 KiB- or 64 KiB-page Linux kernel. On
    x86_64 Linux with 4 KiB pages, both tests fail today under these one-line mutations of the lend
    condition in _relay(), and they should fail under them on large-page kernels too:
    • dropping self._producer_blocked() fails ..._after_a_producer_finishes
    • dropping self._reader._consumer_waiting.is_set() fails ..._consumer_waiting_for_the_terminal
  • make check and make test pass on macOS and x86_64 Linux.

@setarcos offered to test the change on Linux loongarch64 (16 KiB pages).

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions