-
Notifications
You must be signed in to change notification settings - Fork 990
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
[5.0.x] inefficient locking on recv of peers lists can result in failure to get peers lock #3566
[5.0.x] inefficient locking on recv of peers lists can result in failure to get peers lock #3566
Conversation
@@ -127,6 +127,15 @@ impl PeerStore { | |||
batch.commit() | |||
} | |||
|
|||
pub fn save_peers(&self, p: Vec<PeerData>) -> Result<(), Error> { |
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.
would it make sense to also redefine the save_peer
above to delegate the call to save_peers
for a vector of size 1?
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.
That would require every call to save_peer to create a Vec and add the single item. Im not convinced thats better than having a few lines of similar code.
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 think the suggestion is to make save_peer just a wrapper around save_peers, instead of duplicating the batch DB code.
} | ||
to_save.push(peer); | ||
} | ||
if let Err(e) = self.save_peers(to_save) { |
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.
Is there anything wrong if we end up saving no peers because this one failed?
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.
It's not ideal, but how would you know which one failed?
@antiochp I am happy with this change. Please merge or reject when you get a chance. |
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.
This looks good to me. 👍
I'm planning to test this locally and will merge once done.
…ure to get peers lock (mimblewimble#3566) * fix for: inefficient locking of peers lists can result in failure to get peers lock * dont hold the peers Vec lock while writing to the peers lmdb
…ure to get peers lock (#3566) (#3570) * fix for: inefficient locking of peers lists can result in failure to get peers lock * dont hold the peers Vec lock while writing to the peers lmdb Co-authored-by: Blade Doyle <[email protected]>
…t in failure to get peers lock (mimblewimble#3566) (mimblewimble#3570) * fix for: inefficient locking of peers lists can result in failure to get peers lock * dont hold the peers Vec lock while writing to the peers lmdb
see: #3561
This change batches the write of PEER_LIST results into the peers lmdb to increase efficiency and reduce lock timeouts.
20210215 11:28:20.196 ERROR grin_p2p::peers - add_connected: failed to get peers lock