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

transport_service: Improve connection stability by downgrading connections on substream inactivity #260

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

lexnv
Copy link
Collaborator

@lexnv lexnv commented Sep 30, 2024

This PR advances the keep-alive timeout of the transport service.

Previously, the keep-alive timeout was triggered 5 seconds after the connection was reported to the transport service regardless of substream activity.

  • (0secs) T0: connection established; keep-alive timeout set to 5seconds in the future
  • (4secs) T1: substream A, B, C opened
  • (5secs) T2: keep-alive timeout triggered and the connection is downgraded. T1 was not taken into account, otherwise, the keep-alive timeout should be triggered at second 9 (T1 at 4 seconds + keepalive 5 seconds)
  • (6secs) T3: substreams A, B, C closed -> connection closes
  • (7secs) T4: cannot open new substreams even if we expected the connection to be kept alive for longer

In this PR:

  • KeepAliveTracker structure to forward the keep-alive timeout of connections.
  • Connection ID is forwarded to SubstreamOpened events to identify properly substream Ids. This is needed because the ConnectionContext contains up to two connections (primary and secondary)

Testing Done

  • test to ensure keepalive downgrades the connection after 5 seconds
  • test to ensure keepalive is forwarded on substream activity
  • test to ensure a downgraded connection with dropped substreams is closed

Closes #253.

Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv lexnv added the enhancement New feature or request label Sep 30, 2024
@lexnv lexnv self-assigned this Sep 30, 2024
@lexnv lexnv changed the title TransportService: Improve connection stability by downgrading connections on substream inactivity transport_service: Improve connection stability by downgrading connections on substream inactivity Sep 30, 2024
Base automatically changed from lexnv/improve-protocol-logs to master October 1, 2024 14:48
Copy link
Collaborator Author

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

Will have another pass over KeepAliveTracker. Im not entirely happy at the moment that we need a service notification to advance the polling here, will have to see how this plays with low-connectivity environments

Comment on lines +217 to +218
cx.waker().wake_by_ref();
return Poll::Pending;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A simplified example in the rust playground where I've reproduced this: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=182bf56449de7827c8f83b649dbba4fb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant