Differential D19488

TransactionSequence: cancel all subjobs on rollback-from-user.
ClosedPublic
Actions

Authored by dfaure on Mar 3 2019, 11:02 AM.

Details

Reviewers
dvratil
vkrause
Commits
R165:1aace13b8dd6: TransactionSequence: cancel all subjobs on rollback-from-user.
Summary

This replaces commit f14a6e997629 which wasn't the right fix for the
same problem.

Test Plan

F5 in a large folder in kmail, then pressing the Abort button
in the middle of the sync. It used to just block the sync, while now it's
possible to resume afterwards.

Diff Detail

Branch
transaction_stuff
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 9558
Build 9576: arc lint + arc unit
dfaure requested review of this revision.Mar 3 2019, 11:02 AM
dfaure created this revision.
dvratil added a comment.Mar 6 2019, 10:20 PM

You say it /replaces/ the faulty commit, but the code introduced in that commit wasn't actually touched here. Are you going to revert the other one and apply this one?

dvratil accepted this revision.Mar 6 2019, 10:31 PM

Never mind, I got confused by the diffs (and the commit message) - the code from the referenced commit is actually removed in D19487 :) Maybe squash both changes into one?

This revision is now accepted and ready to land.Mar 6 2019, 10:31 PM
dfaure added a comment.Mar 7 2019, 8:03 AM

I kept them separate because D19487 is about fixing what happens after a rollback coming from the DB itself (e.g. deadlock), while D19488 is about a better fix for a rollback coming from the user pressing Cancel.
Two different testcases.

dfaure planned changes to this revision.Mar 7 2019, 8:54 AM

Looking at this more closely, I see that canceling works because.. the resource crashes.
I'll need to spend more time on this, starting with writing a unittest for rollingback an ItemSync.

dfaure updated this revision to Diff 53791.Mar 13 2019, 2:01 PM

Add unittest for cancelling; fix the code until it works...

This revision is now accepted and ready to land.Mar 13 2019, 2:01 PM
dvratil added inline comments.Mar 13 2019, 8:36 PM
src/core/jobs/transactionsequence.cpp
249 ↗(On Diff #53791)

So why not kill the subjobs quietly? Since we are cancelling them, we don't care about their result, right?

dfaure added inline comments.Mar 13 2019, 10:25 PM
src/core/jobs/transactionsequence.cpp
249 ↗(On Diff #53791)

That's one of the things I tried, and it means mPendingJobs isn't decremented in ItemSync so the itemsync never finishes.

(of course Quietly also requires calling KCompositeJob::removeSubjob(job) here in the loop, to avoid keeping dangling jobs in subjobs(), but that's the easy part)

dfaure updated this revision to Diff 53846.Mar 13 2019, 11:32 PM

Simplify diff by removing the extracted helper method that I didn't end up using.

dfaure updated this revision to Diff 53910.Mar 14 2019, 7:05 PM

fix compilation error in last change

dfaure added a comment.Mar 18 2019, 9:41 PM

Dan, do you want to review the updated version? The phabricator status is incorrect, this version hasn't been reviewed.

dvratil accepted this revision.Mar 21 2019, 3:45 PM
dfaure closed this revision.Mar 21 2019, 3:50 PM

Showing Only Differences

This revision modifies 2 more files that are hidden because they were not modified between selected diffs and they have no inline comments.

Revision Contents
Changeset List

Diff 53846

View Options

src/core/jobs/transactionsequence.cpp

Loading...