stopper: Ensure that Closers and Ctxs are always cleaned up #27920
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes cockroachdb#27908. Release note: None
Before this change the Stopper made no guarantee that `Closers`
registered by the `AddCloser` method or `Contexts` tied to
the `Stopper` with `WithCancelOn{Quiesce,Stop}` were ever properly
cleaned up. In cases where these methods were called concurrently
with the Stopper stopping, it was possible for the resources to
leak. This change fixes this by handling cases where these methods
are called concurrently with or after the Stopper has stopped. This
allows them to provide stronger external guarantees.
Release note: None
|
This change is |
benesch
approved these changes
Jul 25, 2018
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 2 of 2 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained (and 1 stale)
|
bors r=benesch |
craig bot
pushed a commit
that referenced
this pull request
Jul 25, 2018
27829: kv,storage: disable pipelining in merge transaction r=nvanbenschoten a=benesch
It turns out that pipelining is fundamentally incompatible with merge
transactions. The extra QueryIntent requests generated by pipelining can
get routed to a RHS that is currently blocked for merging and wedge the
merge. Consider the following sequence of events:
1. The merge transaction begins and lays down intents on the meta2 and
local copy of the RHS range descriptor using asynchronous commits.
2. The RHS loses its lease.
3. The merge transaction sends a GetSnapshotForMergeRequest. The
pipeliner injects two QueryIntent requests, one to the meta2 range
descriptor and one to the local range descriptor on the RHS, as
non-transactional requests always flush the pipeline.
4. The QueryIntent request hits the RHS and triggers a lease
acquisition. The lease acquisition notices that the local range
descriptor has a deletion intent, infers that a merge is in
progress, and blocks all traffic, except for GetSnapshotForMerge
requests.
GetSnapshotForMerge requests are allowed to proceed even when the range
is blocked for merging but QueryIntent requests are not. We can't, in
general, allow QueryIntent requests through, as we might return stale
data if the merge has already committed and the LHS is processing
new writes. We could, I think, allow QueryIntents for the local range
descriptor to proceed, but that's a very-specific corner case and hard
to reason about.
Just disable pipelining in the merge transaction to sidestep the issue.
One other option: we could allow the RHS to serve reads up until the
commit timestamp of the merge transaction. That would allow the
GetSnapshotForMerge and QueryIntent requests from the merge transaction
through, as well as any other read traffic executing beneath the merge
transaction's timestamp. The downside is that the new leaseholder would
have to wait out the full clock offset after the merge txn committed to
serve any writes. (Just like the lease stasis period.) It was decided
that this option was too disruptive and complex to pursue for the time
being.
Fix #27820.
27920: stopper: Ensure that Closers and Ctxs are always cleaned up r=benesch a=nvanbenschoten
Before this change, the Stopper made no guarantee that `Closers`
registered by the `AddCloser` method or `Contexts` tied to
the `Stopper` with `WithCancelOn{Quiesce,Stop}` were ever properly
cleaned up. In cases where these methods were called concurrently
with the Stopper stopping, it was possible for the resources to
leak. This change fixes this by handling cases where these methods
are called concurrently with or after the Stopper has stopped. This
allows them to provide stronger external guarantees.
The PR also correctly synchronizes access to the `sCancels` map to
fix #27908.
Release note: None
Co-authored-by: Nikhil Benesch <nikhil.benesch@gmail.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Build succeeded |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this change, the Stopper made no guarantee that
Closersregistered by the
AddClosermethod orContextstied tothe
StopperwithWithCancelOn{Quiesce,Stop}were ever properlycleaned up. In cases where these methods were called concurrently
with the Stopper stopping, it was possible for the resources to
leak. This change fixes this by handling cases where these methods
are called concurrently with or after the Stopper has stopped. This
allows them to provide stronger external guarantees.
The PR also correctly synchronizes access to the
sCancelsmap tofix #27908.
Release note: None