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.

Sign up for GitHub

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

Jump to bottom

Only call DisconnectNodes once per second #3421

Merged
merged 5 commits into from
Apr 17, 2020
Merged

Only call DisconnectNodes once per second #3421

merged 5 commits into from
Apr 17, 2020

Conversation

codablock
Copy link

See individual commits. This PR also includes "Only run InactivityCheck once per second" from #3420 to avoid future merge conflicts. I'll rebase this PR after #3420 is merged into develop.

@codablock codablock added this to the 16 milestone Apr 16, 2020
@PastaPastaPasta
Copy link
Member

@codablock tests failing

UdjinM6
UdjinM6 reviewed Apr 16, 2020
View reviewed changes
src/net.cpp Outdated Show resolved Hide resolved
@codablock
Copy link
Author

Tests are hopefully fixed now (had to call DeleteNode() without any locks held)

@codablock
Only run DisconnectNodes once per second
30e4afa 
This change is quite invasive for integration tests, as they all expect
connections to be dropped ASAP. The next commits all try to fix the upcoming
issues.
@codablock
Also wait for node1 to close all sockets
0569997
@codablock
Invoke DisconnectNodes right before checking for duplicate connections
76d7b17
@codablock
In disconnect_p2ps(), wait for p2p connections to disappear from getp…
3054854 
…eerinfo()
@codablock
Invoke DisconnectNodes from getconnectioncount/getpeerinfo/getnetwork…
2f1b3a3 
…info

This ensures disconnected nodes don't appear for callers
@codablock codablock mentioned this pull request Apr 17, 2020
Merged
@UdjinM6
Copy link

UdjinM6 commented Apr 17, 2020

vNodes/vNodesDisconnected are also accessed to delete nodes in CConnman::Stop(). Shouldn't we protect them there now that we call DisconnectNodes() not only in ThreadSocketHandler but in many other places? e.g. smth like 3ae02dd

@codablock
Copy link
Author

@UdjinM6 yeah I noticed this as well, but decided to not fix it and leave it the way it is. Bitcoin also never fixed the missing cs_vNodes lock. It's semi-ok in this case, as Stop() first ensures that all network threads are stopped, so there is nothing left that could access the vectors.

@UdjinM6
Copy link

UdjinM6 commented Apr 17, 2020

Stop() first ensures that all network threads are stopped, so there is nothing left that could access the vectors.

Well, we call DisconnectNodes() from various rpc commands also now but as far as I can see RPC is stopped before connman so yeah, should be ok probably but looks a bit scary :)

UdjinM6
UdjinM6 approved these changes Apr 17, 2020
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK

@codablock codablock merged commit 402b139 into dashpay:develop Apr 17, 2020
@codablock codablock deleted the pr_speedups6 branch April 17, 2020 10:23
PastaPastaPasta
PastaPastaPasta reviewed Apr 17, 2020
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

post-utACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PastaPastaPasta PastaPastaPasta PastaPastaPasta left review comments

@UdjinM6 UdjinM6 UdjinM6 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
16
Development

Successfully merging this pull request may close these issues.
3 participants
@codablock @PastaPastaPasta @UdjinM6