Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Prevent database corruption on OOM #2832

Merged
merged 2 commits into from
Oct 24, 2016
Merged

Prevent database corruption on OOM #2832

merged 2 commits into from
Oct 24, 2016

Conversation

arkpar
Copy link
Collaborator

@arkpar arkpar commented Oct 23, 2016

No description provided.

@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Oct 23, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 86.206% when pulling 64b2b88 on db-corruption-fix into 8cf9934 on master.

@@ -417,11 +418,10 @@ impl Database {
};
}

/// Commit buffered changes to database.
pub fn flush(&self) -> Result<(), String> {
/// Commit buffered changes to database. Must be called under `flush_lock`
Copy link
Contributor

Choose a reason for hiding this comment

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

is there some way to ensure it is always called under flush_lock?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's a private function so we can uphold invariants at the module level

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is an internal function. The code in the module only calls under the lock. I could add a try_lock runtime check but this seems an overkill considering we don't have such checks anywhere else in the codebase.

Copy link
Collaborator

@tomusdrw tomusdrw Oct 24, 2016

Choose a reason for hiding this comment

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

Can you pass &mut MutexGuard<bool> as an unused parameter?

Copy link
Contributor

@gavofyork gavofyork Oct 24, 2016

Choose a reason for hiding this comment

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

yup - even for internal functions, it's pretty easy for someone else to come along and mistakenly call it. docs all too easily get out of date. functions that require particular external conditions to act properly (like a member lock being made) are inherently dangerous.

a minimum would be to append _with_lock to the function name, though i'd prefer something that didn't compile unless called properly.

@gavofyork gavofyork added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 24, 2016
@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Oct 24, 2016
@rphmeier rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 24, 2016
@gavofyork gavofyork added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Oct 24, 2016
@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Oct 24, 2016
@gavofyork gavofyork added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 24, 2016
@gavofyork gavofyork merged commit edbd667 into master Oct 24, 2016
@gavofyork gavofyork deleted the db-corruption-fix branch October 24, 2016 16:32
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 86.179% when pulling 1af1962 on db-corruption-fix into 8cf9934 on master.

arkpar added a commit that referenced this pull request Oct 30, 2016
* Prevent database corruption on OOM

* Renamed write_flushing
arkpar added a commit that referenced this pull request Oct 30, 2016
* Prevent database corruption on OOM (#2832)

* Prevent database corruption on OOM

* Renamed write_flushing

* v1.3.10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants