Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(kad): introduce AsyncBehaviour #5294
base: master
Are you sure you want to change the base?
feat(kad): introduce AsyncBehaviour #5294
Changes from 1 commit
9823487
e455bee
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of following the design introduced with
stream
, i.e. introducing a clonableControl
handle which implements the methods, that way we don't need to keep a reference to the
Swarm
to call the methods.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it would probably be great !!
I'm thinking about it but I really don't see how I could implement it since I need to capture the events emitted by the
kad::Behaviour
. Do you have an idea ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jxs , I've thought a little bit about this. The only way I see to make a
Control
work for thekad::Behaviour
is to have the entireBehaviour
behind anArc<Mutex<>>
. I don't know if it is a good or bad idea.Will the "control" semantic become something standard for all the behaviours ? If so why not. But if not, I don't know how it will be understood by the end user to have some behaviours using a
control
and others not.I'd really like to also have your opinion on this @guillaumemichel when you have the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am discovering how
Control
works and I think it would make sense in this case. What would be the arguments against using it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need an Arc/ Mutex.
You could split the current
AsyncBehavior
logic into two parts: the Control part and the NetworkBehavior part, that communicate through an additional mpsc channel (the "command-channel").Control
would have all the async variants of the kademlia behavior (get_closest_peers_async
,get_providers_async
, etc.).There, it would create the mpsc channel for the results, and then send the a tuple (
Command::GetProviders {..}, mpsc::Sender<AsyncQueryResult>)
through the "command-channel" to theAsyncBehavior
.AsyncBehavior
would wrap the kademlia behavior and implNetworkBehavior
. In itspoll_next
loop it would poll the receiving side of the command-channel, and handle the incoming command by calling the matching function on the wrapped kademlia behavior and storing theSender
in thequery_result_senders
hashmap.poll_next
would also still have the already existing logic for interceptingToSwarm::GenerateEvent
events from the inner behavior and forwarding the results.Does that make sense? @jxs was that roughly what you had in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The query could also have been triggered through the non-async methods of the inner kad Behavior that we deref to, right?