Fix a potential bug in DropCloserReadHalf::take()

[steedmicro]

Description

I fixed a potential bug in DropCloserReadHalf::take() function.
The regression test against the potential bug is done by send_and_take_fuzz_test in nativelink-util/tests/buf_channel_test.rs.
Detailed context can be found in the issue link below.

Fixes #578

Type of change

• Bug fix (non-breaking change which fixes an issue)

Checklist

• Updated documentation if needed
• Tests added/amended
• bazel test //... passes locally
• PR is contained in a single commit, using git amend see some docs

---

This change is 

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
nativelink-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 16, 2024 6:54am

[allada]

Reviewable status: 0 of 1 LGTMs obtained

---

nativelink-util/src/buf_channel.rs line 328 at r1 (raw file):

                return Ok(first_chunk);
            }
            assert!(

nit: This assert is useless now.

---

nativelink-util/src/buf_channel.rs line 364 at r1 (raw file):

            output.put(chunk);

            if output.len() == size && self.partial.is_some() {

nit: For readability lets make this:

if self.partial.is_some() {
  assert!(output.len() == size, "If partial is set expected output length to be {size}");
  return Ok(output.freeze());
}

---

nativelink-util/src/buf_channel.rs line 364 at r1 (raw file):

            output.put(chunk);

            if output.len() == size && self.partial.is_some() {

nit: Move this (and the rest of the code below) to the line after loop {

[allada]

Reviewable status: 0 of 1 LGTMs obtained

a discussion (no related file):
Testing this is a bit tricky. I think we should do more of a fuzz test on this.

What I think we should do is make a test that does something like:

for data_size in 0..10 {
  data = random data of size "data_size";
  for read_size in 0..10 {
    for write_size in 0..10 {
      let (tx, rx) = BuffchannelPair();
      let send_fut = async move {
        .. .send() "data" in "write_size" chunks.
      };
      let recv_fut = async move {
        .. .take() in "read_size" chunks.
      };
      let (send_res, recv_res) = join!(send_fut, recv_fut);
      ... check results and ensure the receiver got all the data and matches "data".
    }
  }
}

---

[steedmicro]

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), pre-commit-checks, publish-image, ubuntu-20.04, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable

a discussion (no related file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

Testing this is a bit tricky. I think we should do more of a fuzz test on this.

What I think we should do is make a test that does something like:

for data_size in 0..10 {
  data = random data of size "data_size";
  for read_size in 0..10 {
    for write_size in 0..10 {
      let (tx, rx) = BuffchannelPair();
      let send_fut = async move {
        .. .send() "data" in "write_size" chunks.
      };
      let recv_fut = async move {
        .. .take() in "read_size" chunks.
      };
      let (send_res, recv_res) = join!(send_fut, recv_fut);
      ... check results and ensure the receiver got all the data and matches "data".
    }
  }
}

Done.

---

[allada]

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), ubuntu-20.04, ubuntu-22.04

a discussion (no related file):
Can you also verify if this new test fails without the other changes merged?

---

---

nativelink-util/tests/buf_channel_test.rs line 229 at r2 (raw file):

        for data_size in 1..11 {
            // Create a random bytes of `data_size`.
            let data: Vec<u8> = std::iter::repeat_with(|| rand::random::<u8>())

nit: Lets not make random, instead use a const array of size "11", then slice/copy it.

---

nativelink-util/tests/buf_channel_test.rs line 236 at r2 (raw file):

                for read_size in 1..11 {
                    let tx_data = Bytes::from(data.clone());
                    let rx_data = Bytes::from(data.clone());

nit: Rename this expected_data.

---

nativelink-util/tests/buf_channel_test.rs line 249 at r2 (raw file):

                    };
                    let rx_fut = async move {
                        let mut rx_mut_data = bytes::BytesMut::new();

nit: import BytesMut above, try not to use inline crate names.

---

nativelink-util/tests/buf_channel_test.rs line 249 at r2 (raw file):

                    };
                    let rx_fut = async move {
                        let mut rx_mut_data = bytes::BytesMut::new();

nit: Rename this round_trip_data

[steedmicro]

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), pre-commit-checks, publish-image, ubuntu-20.04, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable

a discussion (no related file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

Can you also verify if this new test fails without the other changes merged?

The previous buf_channel.rs fails the newly written test - send_and_take_test.
It's clear that our current updates are fixing a bug.

---

[allada]

Reviewed 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Remote / large-ubuntu-22.04

---

nativelink-util/tests/buf_channel_test.rs line 227 at r3 (raw file):

    #[tokio::test]
    async fn send_and_take_test() -> Result<(), Error> {

nit: Please add a reference to the ticket this test is a regression test against.

---

nativelink-util/tests/buf_channel_test.rs line 227 at r3 (raw file):

    #[tokio::test]
    async fn send_and_take_test() -> Result<(), Error> {

nit: rename this send_and_take_fuzz_test()

---

nativelink-util/tests/buf_channel_test.rs line 228 at r3 (raw file):

    #[tokio::test]
    async fn send_and_take_test() -> Result<(), Error> {
        for data_size in 1..11 {

nit: make 11 less magical and make it const DATA3_END_POS: usize = DATA3.len() + 1;