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

await on join #2137

Merged
merged 6 commits into from Feb 28, 2020
Merged

await on join #2137

merged 6 commits into from Feb 28, 2020

Conversation

@freesig
Copy link
Contributor

freesig commented Feb 28, 2020

PR summary

This awaits on the new connection future instead of just spawning it because otherwise the Lib3hToClient::HandleGetGossipingEntryList and Lib3hToClient::HandleGetAuthoringEntryList messages get sent and can be responded to before the join is complete. This leads to the responses to those messages being thrown away. Then the agents id's never actually get into the DHT.

testing/benchmarking notes

( if any manual testing or benchmarking was/should be done, add notes and/or screenshots here )

followups

( any new tickets/concerns that were discovered or created during this work but aren't in scope for review here )

changelog

  • if this is a code change that effects some consumer (e.g. zome developers) of holochain core, then it has been added to our between-release changelog with the format
- summary of change [PR#1234](https://github.com/holochain/holochain-rust/pull/1234)

documentation

Copy link
Member

lucksus left a comment

👏 👏 👏

let f = self.new_connection(space_hash, agent_id, uri);
tokio::task::spawn(f);
f.await

This comment has been minimized.

Copy link
@neonphog

neonphog Feb 28, 2020

Contributor

My intention was that functions prefixed with spawn_ would not return futures (or anything else) because they spawn the task.

If we don't need this, we can just delete this whole function - and lib.rs can call new_connection() directly : )

@@ -522,17 +531,20 @@ fn spawn_handle_message_hello(
}
}

fn spawn_handle_message_join_space(
async fn spawn_handle_message_join_space(

This comment has been minimized.

Copy link
@neonphog

neonphog Feb 28, 2020

Contributor

similarly, remove the spawn_ prefix here if that's not what we're doing.

Copy link
Contributor

neonphog left a comment

couple cosmetic name change suggestions - but this looks awesome : )

Nice find! 👍

@zippy
zippy approved these changes Feb 28, 2020
@zippy
zippy approved these changes Feb 28, 2020
Copy link
Member

zippy left a comment

Approved with the naming changes so that we keep functions consistently named to intent. WOOT!

crates/sim2h/src/lib.rs Outdated Show resolved Hide resolved
neonphog and others added 4 commits Feb 28, 2020
Co-Authored-By: Connor Turland <connorturland@gmail.com>
crates/sim2h/src/lib.rs Outdated Show resolved Hide resolved
@zippy zippy merged commit 79fd68a into develop Feb 28, 2020
6 checks passed
6 checks passed
ci/circleci: app-spec-tests-sim2h Your tests passed on CircleCI!
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: cli-tests Your tests passed on CircleCI!
Details
ci/circleci: fmt Your tests passed on CircleCI!
Details
ci/circleci: stress-tests-sim2h Your tests passed on CircleCI!
Details
ci/circleci: wasm-conductor-tests Your tests passed on CircleCI!
Details
@neonphog neonphog deleted the fix_5000 branch Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.