Skip to content

Commit

Permalink
swarm/one_shot: Initialize handler with KeepAlive::Until (#1698)
Browse files Browse the repository at this point in the history
* swarm/one_shot: Add test for not keeping alive idle connection

A `OneShotHandler` without any ongoing requests should not keep the
underlying connection alive indefinitely.

* swarm/one_shot: Initialize handler with KeepAlive::Until

The `OneShotHandler` `keep_alive` property is altered on incoming and
outgoing reqeusts. By default it is initialized in `KeepAlive::Yes`. In
case there are no incoming or outgoing requests happening, this state is
never changed and thus the handler keeps the underlying connection alive
indefinitely.

With this commit the handler is initialized with `KeepAlive::Until`. As
before the `keep_alive` timer is updated on incoming requests and set to
`KeepAlive::Yes` on outgoing requests.

* swarm/one_shot: Move KeepAlive logic to poll

A `ProtocolsHandler` can be created before the underlying connection is
established. Thus setting a keep alive timeout might be problematic.
Instead set `keep_alive` to `Yes` at construction and alter it within
`ProtocolsHandler::poll`.

* swarm/CHANGELOG: Add entry for OneShotHandler keep-alive
  • Loading branch information
mxinden authored Aug 13, 2020
1 parent 9eca739 commit e32ff74
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 5 deletions.
3 changes: 3 additions & 0 deletions swarm/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ callback.

- Update the `libp2p-core` dependency to `0.21`, fixing [1584](https://github.com/libp2p/rust-libp2p/issues/1584).

- Fix connections being kept alive by `OneShotHandler` when not handling any
requests [PR 1698](https://github.com/libp2p/rust-libp2p/pull/1698).

# 0.20.1 [2020-07-08]

- Documentation updates.
Expand Down
36 changes: 31 additions & 5 deletions swarm/src/protocols_handler/one_shot.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,11 +161,6 @@ where
_: Self::OutboundOpenInfo,
) {
self.dial_negotiated -= 1;

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

self.events_out.push(out.into());
}

Expand Down Expand Up @@ -221,6 +216,10 @@ where
}
} else {
self.dial_queue.shrink_to_fit();

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

Poll::Pending
Expand All @@ -245,3 +244,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(_)));
}
}

0 comments on commit e32ff74

Please sign in to comment.