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(relay): flush relayed connection once idle #3765

Merged
merged 6 commits into from
Apr 11, 2023

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented Apr 11, 2023

Description

As a relay, when forwarding data between relay-connection-source and -destination and vice versa, flush write side when read currently has no more data available.

Notes & open questions

Credit goes to @MarcoPolo for discovering this!

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

As a relay, when forwarding data between relay-connection-source and -destination and vice versa,
flush write side when read currently has no more data available.
use quickcheck::QuickCheck;
use std::io::ErrorKind;
use std::pin::Pin;
use std::task::{Context, Poll};
use std::time::Duration;

struct Connection {
Copy link
Member Author

@mxinden mxinden Apr 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make confusing diff easier to read:

  • Connection is only used in quickckeck test, thus moved into quickcheck test.
  • PendingConnection is only used in max_circuit_duration test, thus moved into max_circuit_duration test.

protocols/relay/CHANGELOG.md Outdated Show resolved Hide resolved
protocols/relay/src/copy_future.rs Outdated Show resolved Hide resolved

#[test]
fn forward_data_should_flush_on_pending_source() {
struct NeverEndingSource {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
struct NeverEndingSource {
struct NeverEndingStory {

Comment on lines +383 to +385
"Given that destination is wrapped with a `BufWrite`, the write doesn't (yet) make it to \
the destination. The source might have more data available, thus `forward_data` has not \
yet flushed.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Given that destination is wrapped with a `BufWrite`, the write doesn't (yet) make it to \
the destination. The source might have more data available, thus `forward_data` has not \
yet flushed.",
"Expect BufWriter to still have space and thus not forward the poll_write call",

protocols/relay/src/copy_future.rs Outdated Show resolved Hide resolved
@thomaseizinger thomaseizinger changed the title fix(relay): flush write on pending read fix(relay): flush relayed connection once idle Apr 11, 2023
mxinden and others added 3 commits April 11, 2023 13:01
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
@mxinden
Copy link
Member Author

mxinden commented Apr 11, 2023

@mxinden
Copy link
Member Author

mxinden commented Apr 11, 2023

//CC @p-shahi for visibility for the universal connectivity project.

@mergify mergify bot merged commit eeddf27 into libp2p:master Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants