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

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

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

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
1454–1457

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
1454–1457

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.Fri, Jan 26, 2:42 PM

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.