Allow columns to be marked as nonmutable (so save() will not change them)
ClosedPublic
Actions

Authored by epriestley on Feb 19 2015, 3:32 PM.

Details

Reviewers

btrahan

Maniphest Tasks

T6840: Repository imported with some commits missing Herald step

Commits

Restricted Diffusion Commit
rP29fd3f136b88: Allow columns to be marked as nonmutable (so save() will not change them)

Summary

Ref T6840. This feels a little dirty; open to alternate suggestions.

We currently have a race condition where multiple daemons may load a commit and then save it at the same time, when processing "reverts X" text. Prior to this feature, two daemons would never load a commit at the same time.

The "reverts X" load/save has no effect (doesn't change any object properties), but it will set the state back to the loaded state on save(). This overwrites any flag updates made to the commit in the meantime, and can produce the race in T6840.

In other cases (triggers, harbormaster, repositories) we deal with this kind of problem with "append-only-updates + single-consumer", or a bunch of locking. There isn't really a good place to add a single consumer for commits, since a lot of daemons need to access them. We could move the flags column to a separate table, but this feels pretty complicated. And locking is messy, also mostly because we have so many consumers.

Just exempting this column (which has unusual behavior) from save() feels OK-ish? I don't know if we'll have other use cases for this, and I like it even less if we never do, but this patch is pretty small and feels fairly understandable (that said, I also don't like that it can make some properties just silently not update if you aren't on the lookout).

So, this is a fix, and feels simplest/least-bad for the moment to me, I thiiink.

Test Plan

Added and executed unit tests.

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley updated this revision to Diff 28493.Feb 19 2015, 3:32 PM

epriestley retitled this revision from to Allow columns to be marked as nonmutable (so save() will not change them).

epriestley updated this object.

epriestley edited the test plan for this revision. (Show Details)

epriestley added a reviewer: btrahan.

epriestley added a task: T6840: Repository imported with some commits missing Herald step.

Herald added a subscriber: epriestley. · View Herald TranscriptFeb 19 2015, 3:32 PM

Another fix would be to try to let transactions say "this transaction did not affect the main object's state, so the object does not need to be save()d". Then default edge transactions (including the "x reverts y" mention-style transaction) could set this flag, and we'd eventually skip the save() in the TransactionEditor, and then moot the race.

That feels way more complicated to me, though, and really easy to forget about and cause an unrelated, race-conditioney break. This isn't the most obvious mechanism, but you generally can't test your code properly without discovering that you missed it. With implicit "no save needed", it would be very easy to overlook that you caused a save() to occur and triggered a race.

epriestley added a child revision: D11823: Set "importStatus" as nonmutable on save().Feb 19 2015, 3:41 PM

Seems okay to me, particularly with a little more 'chilling effect' in the doc language. Doesn't feel dirty to me persay since Lisk itself is this magical little thing.

My concern with this would be engineers over-using this sort of thing and its not really a practical one with our small company. :D In that vein, I think the proposed alternate solution via the editor sounds more prone to error too.