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.

Sign up for GitHub

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

Jump to bottom

swarm/one_shot: Initialize handler with KeepAlive::Until #1698

Merged
merged 5 commits into from
Aug 13, 2020
Merged

swarm/one_shot: Initialize handler with KeepAlive::Until #1698

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1edbd67
swarm/one_shot: Add test for not keeping alive idle connection
mxinden Aug 12, 2020
41e310f
swarm/one_shot: Initialize handler with KeepAlive::Until
mxinden Aug 12, 2020
b409252
swarm/one_shot: Move KeepAlive logic to poll
mxinden Aug 13, 2020
cdfe1c7
Merge branch 'libp2p/master' into oneshot-keepalive
mxinden Aug 13, 2020
cc04b5b
swarm/CHANGELOG: Add entry for OneShotHandler keep-alive
mxinden Aug 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 28 additions & 1 deletion swarm/src/protocols_handler/one_shot.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ where
dial_queue: SmallVec::new(),
dial_negotiated: 0,
max_dial_negotiated: 8,
keep_alive: KeepAlive::Yes,
keep_alive: KeepAlive::Until(Instant::now() + config.keep_alive_timeout),
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be better to just move this condition from inject_fully_negotiated_outbound

        if self.dial_negotiated == 0 && self.dial_queue.is_empty() {
            self.keep_alive = KeepAlive::Until(Instant::now() + self.config.keep_alive_timeout);
        }

into poll()

       ...
       if !self.dial_queue.is_empty() {
            ...
        } else {
            self.dial_queue.shrink_to_fit();
            if self.keep_alive.is_yes() && self.dial_negotiated == 0 {
                self.keep_alive = KeepAlive::Until(Instant::now() + self.config.keep_alive_timeout);
            }
        }

The reason being that starting timers on creation of connection handlers is mildly problematic, because at least right now, handlers are created before the connection is even established, whereas a connection handler is poll()ed as soon as it is established and it gives the handler a chance to "make its first step" before the timeout starts (if there is indeed nothing that the handler wishes to do, as determined by poll()).

config,
}
}
Expand Down Expand Up @@ -245,3 +245,30 @@ impl Default for OneShotHandlerConfig {
}
}

#[cfg(test)]
mod tests {
use super::*;

use futures::executor::block_on;
use futures::future::poll_fn;
use libp2p_core::upgrade::DeniedUpgrade;
use void::Void;

#[test]
fn do_not_keep_idle_connection_alive() {
let mut handler: OneShotHandler<_, DeniedUpgrade, Void> = OneShotHandler::new(
SubstreamProtocol::new(DeniedUpgrade{}),
Default::default(),
);

block_on(poll_fn(|cx| {
loop {
if let Poll::Pending = handler.poll(cx) {
return Poll::Ready(())
}
}
}));

assert!(matches!(handler.connection_keep_alive(), KeepAlive::Until(_)));
}
}