-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Reset peers.json if the content is not loadable #405
Conversation
I don't think that this is the right fix. |
disagree. the corollary to "fail fast" is "fix fast". if you spot an invalid/old/stale config file, you should remove it immediately, not rely on a side-effect (i.e. overwriting) later. |
PeersStorage::Memory(MemoryPeerstore::empty()) | ||
; peers file will be reset", path); | ||
fs::remove_file(&path).unwrap(); | ||
while Path::new(&path).exists() { |
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 this loop really necessary?
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.
fs::remove_file
may, or not, be performed synchronously, this seems to be OS dependant. On my machine (OSX), this loop is useless. It may not be the case for other systems.
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'm a bit afraid that this will deadlock if the user's system decides to do some black magic, but maybe I'm not rational.
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 find that paranoically-healthy @tomaka :)
I guess I could add some sort of timeout. I am not sure which OS we target but an option is also to check that it works for OSX, Linux and Windows and remove the loop entirely.
Your take on that one, just tell me.
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 - i would wait at most a second then continue anyway
; peers won't be saved", path); | ||
PeersStorage::Memory(MemoryPeerstore::empty()) | ||
; peers file will be reset", path); | ||
fs::remove_file(&path).unwrap(); |
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.
codebase style is to use expect
and prove that the unwrap will not trigger
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.
(in this case, I'm not sure that can be proven. you should try to handle the error somehow, maybe by logging 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.
ok I will adjust
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.
in this case you could either propagate the error (the return type is a Result
) or just continue (since it's non-fatal anyway). i don't have a preference.
Fixed some whitespace. |
} | ||
|
||
if let Ok(peerstore) = JsonPeerstore::new(path.clone()) { | ||
debug!("peers.json reset"); |
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.
Note that this also happens if the file doesn't exist in the first place, ie. if the user starts polkadot for the first time, so the message is not totally correct.
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.
If the file does not exist in the first place, the first test L179 will kick in and the message you mention L196 will never show up.
NodeStore::Json(peerstore) | ||
} else { | ||
warn!(target: "sub-libp2p", | ||
"Failed to reset peer storage {:?}; peers change will not be saved", |
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 can also happen if the user doesn't have the permission to open the file, so the message should just be Failed to open peer storage
IMO.
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 not the message L182 already say that?
I can change the message, no problem. We would then have 2 identical messages. Would you prefer that?
If the file cannot be open because of permissions, this should be caught by the first tests. The message Failed to reset peer storage
really comes second, only after we tried deleting and recreating the file.
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.
Message logic...
* master: (86 commits) Make contract a separate runtime module (#345) Version bump (#450) DB-based blockchain data cache for light nodes (#251) Update libp2p again (#445) Update version on git head change (#444) Fix the public key of bootnode 3 (#441) Update libp2p (#442) Switch to the master branch of libp2p (#427) Export ws port 9944 and add doc (#440) Iterate over overlay to decide which keys to purge (#436) Exit signal gets its own trait (#433) Add docker image (#375) Reset peers.json if the content is not loadable (#405) Limit number of incoming connections (#391) Fix memory leaks in libp2p (#432) Do not queue empty blocks set for import (#431) 5 random fixes (#1) (#435) Chore: fix typo (#434) Prevent building invalid blocks (#430) Better logging for public key mismatch (#429) ...
Release preparation
Fix #404
Tested manually the following cases:
peers.json
peers.json
peers.json