Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kvserver: sync txn record cleanup can be cancelled prematurely #64868

Closed
erikgrinaker opened this issue May 7, 2021 · 1 comment · Fixed by #64869
Closed

kvserver: sync txn record cleanup can be cancelled prematurely #64868

erikgrinaker opened this issue May 7, 2021 · 1 comment · Fixed by #64869
Assignees
Labels
A-kv-transactions Relating to MVCC and the transactional model. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@erikgrinaker
Copy link
Contributor

erikgrinaker commented May 7, 2021

As described in #60585 (comment), when IntentResolver.CleanupTxnIntentsAsync() is called with allowSyncProcessing=true, the cleanup task can be run synchronously and using the caller's context. However, this cleanup task spawns off another async task to clean up the transaction record:

// Run transaction record GC outside of ir.sem.
return ir.stopper.RunAsyncTask(
ctx,
"storage.IntentResolver: cleanup txn records",
func(ctx context.Context) {
err := ir.gcTxnRecord(ctx, rangeID, txn)

In the synchronous case, this cleanup task will be connected to the caller's context, but runs async. As such, the call will likely return to the caller before gcTxnRecord() completes, and the caller may the cancel the context (due to e.g. defer cancel() or disconnection) which will cancel gcTxnRecord() as well.

@erikgrinaker erikgrinaker added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-kv-transactions Relating to MVCC and the transactional model. labels May 7, 2021
@erikgrinaker erikgrinaker self-assigned this May 7, 2021
@nvanbenschoten
Copy link
Member

This is a nice find. @lunevalex should be aware of it when touching this code in #59881. In that change, I think we'll want to pass a Background context to SendAsync so that we don't hit the same kind of issue as the one described here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-kv-transactions Relating to MVCC and the transactional model. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants