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

Allow customizing the delay before closing a Kademlia connection #1477

Merged
merged 7 commits into from
Mar 19, 2020

Conversation

tomaka
Copy link
Member

@tomaka tomaka commented Feb 28, 2020

Reduces from 10 to 2 seconds the delay between the moment we open a connection or receive a response and the moment Kademlia considers the connection useless.

This means that when a connection is open, the Kademlia behaviour has 2 seconds to send its pending query to the node. If the CPU is busy, this time could be too low, but 2 seconds seems large enough for me.

Similarly, after we have received a response, the Kademlia behaviour has 2 seconds to send another query if it wants to reuse the same connection. To me it is actually quite unlikely that there is another query to perform, but also 2 seconds isn't a lot.

I don't think this change is very impactful, but I see this as a positive change in the absolute.

@tomaka tomaka requested a review from romanb February 28, 2020 09:47
@romanb
Copy link
Contributor

romanb commented Mar 2, 2020

This change seems a bit arbitrary and the motivation unclear to me. Since established connections with fully-negotiated protocols, including authenticated encryption, are not particularly cheap to set up, connection reuse is generally something to be desired, especially when using Kademlia over TCP. And since a node's buckets rarely contain more than a few hundred nodes and every iterative query starts with the k closest nodes to the key from these buckets, it is also practically relevant. So providing a time window for connection reuse seems very desirable to me and I already find 10 seconds quite low. Of course, this is primarily a trade-off in memory vs latency, but 2 seconds really does not seem like a good choice to me.

[..] To me it is actually quite unlikely that there is another query to perform.

I think to the contrary that any Kademlia node with a non-negligible amount of DHT traffic always has at least a couple of iterative queries ongoing at any time and due to the fact that all such queries start with nodes from the local buckets a certain overlap in the peers being queried, especially at the early stages of the queries can be assumed (the further the queries iterate towards the target, the more the peers spread out, of course).

How about we make this timeout configurable, leaving the default as it currently is? That would on the one hand allow use-cases that you may have in mind which aggressively optimise for low memory / number of idle connections at the cost of potential increased query / request latency and higher connection churn. On the other hand it would avoid surprises for others as the default won't change.

@tomaka
Copy link
Member Author

tomaka commented Mar 9, 2020

I reworked the pull request to make it configurable.

I added an implementation of IntoProtocolsHandler, as modifying this parameter on an existing handler could introduce ambiguities.

Copy link
Contributor

@romanb romanb left a comment

Choose a reason for hiding this comment

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

I find the code duplication and delegation between KademliaHandler and KademliaHandlerProto unfortunate, considering that it will only grow as more configuration options are added. The fact that the IntoProtocolsHandler::into_handler implementation furthermore does not use any of the parameters given to it is an indication to me that it is maybe not the best approach to go through this abstraction if one merely wants to have the handler construction uniformly configurable for all connections. What do you think about something like:

pub struct KademliaHandlerConfig {
    protocol_config: KademliaProtocolConfig,
    allow_listening: bool,
    idle_timeout: Duration
}

with either public fields or a builder-like API and Default impl and then something like

pub struct KademliaHandler<TUserData> {
    config: KademliaHandlerConfig,
    ...
}

impl<TUserData> KademliaHandler<TUserData> {
    pub fn new(config: KademliaHandlerConfig) -> Self {
        ...
    }
}

?

protocols/kad/src/behaviour.rs Outdated Show resolved Hide resolved
protocols/kad/src/behaviour.rs Outdated Show resolved Hide resolved
@tomaka tomaka changed the title Reduce the delay before closing a Kademlia connection Allow customizing the delay before closing a Kademlia connection Mar 19, 2020
tomaka and others added 4 commits March 19, 2020 13:33
Co-Authored-By: Roman Borschel <romanb@users.noreply.github.com>
Co-Authored-By: Roman Borschel <romanb@users.noreply.github.com>
@tomaka
Copy link
Member Author

tomaka commented Mar 19, 2020

Ready for review again!

@tomaka tomaka merged commit 439dc82 into libp2p:master Mar 19, 2020
@tomaka tomaka deleted the reduce-kad-delay branch March 19, 2020 16:01
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