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

Approval Voting: Refactor Actions to be less racy and fragile #3311

Closed
rphmeier opened this issue Jun 19, 2021 · 1 comment · Fixed by #3366
Closed

Approval Voting: Refactor Actions to be less racy and fragile #3311

rphmeier opened this issue Jun 19, 2021 · 1 comment · Fixed by #3366
Assignees
Labels
I8-refactor Code needs refactoring.

Comments

@rphmeier
Copy link
Contributor

rphmeier commented Jun 19, 2021

Problem Statement

At the moment, in approval voting, we use a system of Actions for logic to trigger writes to the DB within the smaller functions that perform the actual logic of reading from the DB. The problem is that a function may be called a second time before the actions of the first time have been written, and therefore read outdated data from the DB and produce an incoherent write. More generally, the can arise when calling functions A and B successively without applying the Actions from A before calling B.

In minimized pseudocode:

enum Action {
  WriteThing(Thing),
}

impl Db {
 fn read_thing(&self) -> Thing { .. }
 fn write_thing(&mut self, Thing) { .. }
}

fn update_thing(Thing) -> Thing { .. }
fn apply_actions(&mut Db, Iterator<Action>) { .. }

fn make_actions(db: &Db) -> Vec<Action> {
    let thing = db.read_thing();
    vec![Action::WriteThing(update_thing(thing))]
}

fn bad() {
  let mut db = ..;
  let actions_a = make_actions(&db);
  let actions_b = make_actions(&db); // oops, this is based on outdated state.
  apply_actions(&mut db, actions_a.chain(actions_b));
}

The Backend pattern used in #3277 represents a pretty good fix. Instead of passing around a DbReader and producing actions to be put into a DbWriter, we instead pass around an OverlayedBackend which incrementally will build up a set of operations to be committed at the end. We should do the same here, but also be aware that the approval-db may be going away soon so our backend trait may need to become async so we can fetch the data from a dedicated DB subsystem.

@rphmeier rphmeier added the I8-refactor Code needs refactoring. label Jun 19, 2021
@rphmeier rphmeier changed the title Approval Voting: Affine, invariant type to prevent DB state mismatches Approval Voting: Refactor Actions to be less racy and fragile Jun 21, 2021
@Lldenaurois
Copy link
Contributor

Beginning work on this refactor now.

This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
I8-refactor Code needs refactoring.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants