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.
is this loop really necessary?
There was a problem hiding this comment.
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.
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.
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.
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.
codebase style is to use expect and prove that the unwrap will not trigger
There was a problem hiding this comment.
(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.
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.
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.
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.
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.
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.
* 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.jsonpeers.jsonpeers.json