Differential D4065

[RFC] rts/STM: Ensure that invariants are unlocked if transaction commit fails
AbandonedPublic
Actions

Authored by bgamari on Oct 3 2017, 9:16 PM.

Details

Reviewers
erikd
simonmar
fryguybob
austin
Trac Issues
#14310
Summary

While looking in to Trac #14310 I noticed that invariants are not unlocked if a we fail to commit a transaction. I suspect this is the correct fix, although it would be good to get some more experienced eyes on this. Unfortunately this doesn't fix the assertion failure noted in Trac #14310.

Test Plan

TODO

Diff Detail

bgamari created this revision.Oct 3 2017, 9:16 PM
bgamari updated this revision to Diff 14259.Oct 3 2017, 9:37 PM

Add note of clarification

bgamari updated this revision to Diff 14260.Oct 3 2017, 9:39 PM

Fix indentation

bgamari retitled this revision from rts/STM: Ensure that invariants are unlocked if transaction commit fails to [RFC] rts/STM: Ensure that invariants are unlocked if transaction commit fails.Oct 3 2017, 9:52 PM
bgamari edited the summary of this revision. (Show Details)
bgamari updated this revision to Diff 14262.Oct 3 2017, 10:18 PM

Also disconnect touched invariants

bgamari updated this revision to Diff 14263.Oct 3 2017, 10:56 PM

Ensure that we don't commit a transaction unless we are able to lock all invariants

bgamari updated this revision to Diff 14265.Oct 3 2017, 11:00 PM

Split out invariant lock failure fix

bgamari added a comment.Oct 3 2017, 11:12 PM

This alone doesn't fix Trac #14310; for the other piece see D4067.

fryguybob requested changes to this revision.Oct 4 2017, 8:42 PM
fryguybob added inline comments.
rts/STM.c
1457–1460

Disconnecting here is not the right thing to do. I think we just want the unlock_inv.

This revision now requires changes to proceed.Oct 4 2017, 8:42 PM
bgamari updated this revision to Diff 14277.Oct 4 2017, 8:51 PM

Don't disconnect invariants

fryguybob accepted this revision.Oct 4 2017, 8:55 PM

Looks good to me.

This revision is now accepted and ready to land.Oct 4 2017, 8:55 PM
bgamari added inline comments.Oct 4 2017, 9:07 PM
rts/STM.c
1457–1460

Unfortunately it seems that disconnecting is the bit that "fixed" Trac #14310.

bgamari planned changes to this revision.Oct 4 2017, 9:11 PM

Alright, well, it looks like this needs more work afterall.

bgamari requested review of this revision.Oct 5 2017, 6:41 PM

The missing bit can be found in D4073.

This revision is now accepted and ready to land.Oct 5 2017, 6:41 PM
austin resigned from this revision.Nov 9 2017, 5:41 PM
bgamari abandoned this revision.Jan 26 2018, 2:42 PM
bgamari mentioned this in D4073: rts/STM: Fix nested transaction assertion.
bgamari mentioned this in D4067: rts/STM: Commit tx only if we can lock all affected invariants.

Unfortunately D4073, D4067, and D4065 weren't quite sufficient to resolve the issue in question. Howver, ultimately we will likely resolve to deprecate the STM invariant mechanism in GHC 8.6. I won't be continuing work on this issue.

Revision Contents
Changeset List

PathPackages
Mrts/STM.c (14 lines)

Diff 14259

View Options

rts/STM.c

Loading...