Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(torture): handle large overflow values #711

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

gabriele-0201
Copy link
Contributor

Spawning a blocking task to handle the reception of the message is a workaround to make it possible to handle large overflow values.
Using the standard timeout(self.shared.timeout, rx).await??; does not work, it hangs when the expected value to be received is larger than 15KiB.

The problem seems to be strictly related to the interaction between tokio::time::timeout and tokio::sync::oneshot::Receiver.

A simple code like the following perfectly works.

      let message = loop {
          tokio::time::sleep(Duration::from_millis(1)).await;
          if let Ok(something) = rx.try_recv() {
              break something;
          }
      };

It seems that the Future implementation of tokio::sync::oneshot::Receiver switches from being non-blocking to blocking if the size of the message gets bigger than 15KiB.

But I also found this, which is a bug within the timeout function in a couple of versions ago (solved in 1.38.1, we are in 1.42), maybe it is not entirely solved.

Copy link
Contributor

Workarounds is fine, but please please please, leave a note in the code explaining what they are doing.

If you don't do it, the next generation of maintainers (that might include yourself), will definitely get the "wtf is this shit" question, and they will try to remove this code, only to uncover this is for a reason. Another option is that some of us will remember that this is some workaround, but will not remember why, and as such will be reluctant to remove this workaround even when it's not needed.

I do not care about this particular case that much, I care about us aligning on the common approach writing and documenting code.

@gabriele-0201
Copy link
Contributor Author

You are totally right! I really thought I had added the same text both in the pr description and in the code!
I will fix it right now!

Copy link
Contributor

rphmeier commented Jan 23, 2025

Merge activity

  • Jan 23, 2:16 PM EST: A user started a stack merge that includes this pull request via Graphite.
  • Jan 23, 2:22 PM EST: Graphite rebased this pull request as part of a merge.
  • Jan 23, 2:22 PM EST: A user merged this pull request with Graphite.

@rphmeier rphmeier changed the base branch from gm_torture_update_some_names to graphite-base/711 January 23, 2025 19:18
@rphmeier rphmeier changed the base branch from graphite-base/711 to master January 23, 2025 19:20
@rphmeier rphmeier force-pushed the gm_torture_big_overflow_value_bug branch from 0fd1f96 to 9ebcf8d Compare January 23, 2025 19:21
@rphmeier rphmeier merged commit 6bdc70b into master Jan 23, 2025
8 checks passed
@rphmeier rphmeier deleted the gm_torture_big_overflow_value_bug branch January 23, 2025 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants