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

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

Merged
merged 5 commits into from
Aug 13, 2020

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented Aug 12, 2020

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.


Related to paritytech/polkadot#1544.

A `OneShotHandler` without any ongoing requests should not keep the
underlying connection alive indefinitely.
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.
@@ -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()).

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`.
@mxinden
Copy link
Member Author

mxinden commented Aug 13, 2020

Thanks @romanb for pointing out the early initialization issue! Could you take another look?

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.

2 participants