-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
storage/pebbleiter: guard against double-closed Iterators
Replace direct uses of the `*pebble.Iterator` type with a `pebbleiter.Iterator` type definition. In production builds, this type is a simple type definition for `*pebble.Iterator` which suffers no overhead. In `crdb_test` builds, the `pebbleiter.Iterator` is defined as a `*pebbleiter.AssertionIter` type that can assert proper use of the `*pebble.Iterator` type and its contract. Initially, use this new assertion iterator to ensure that a `*pebble.Iterator` is never double-closed, which may cause the Iterator to be added to a sync pool twice leading to races. This type of assertion cannot be done by `*pebble.Iterator`, because the iterator may already be reused by the time the second Close is run. In the future, I'd like to also introduce assertions that pkg/storage always checks validity before calling HasPointAndRange and RangeKeyChanged (see cockroachdb/pebble#2205). Release note: None Epic: None Close #90643.
- Loading branch information
Showing
14 changed files
with
191 additions
and
16 deletions.
There are no files selected for viewing
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
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
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
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
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| load("//build/bazelutil/unused_checker:unused.bzl", "get_x_data") | ||
| load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") | ||
|
|
||
| # gazelle:exclude gen-crdb_test_off.go | ||
| # gazelle:exclude gen-crdb_test_on.go | ||
|
|
||
| # keep | ||
| go_library( | ||
| name = "pebbleiter", | ||
| srcs = select({ | ||
| "//build/toolchains:crdb_test": [":gen-crdb-test-on"], | ||
| "//conditions:default": [":gen-crdb-test-off"], | ||
| }), | ||
| importpath = "github.com/cockroachdb/cockroach/pkg/storage/pebbleiter", | ||
| visibility = ["//visibility:public"], | ||
| deps = ["@com_github_cockroachdb_pebble//:pebble"], | ||
| ) | ||
|
|
||
| REMOVE_GO_BUILD_CONSTRAINTS = "cat $< | grep -v '//go:build' | grep -v '// +build' > $@" | ||
|
|
||
| genrule( | ||
| name = "gen-crdb-test-on", | ||
| srcs = ["crdb_test_on.go"], | ||
| outs = ["gen-crdb_test_on.go"], | ||
| cmd = REMOVE_GO_BUILD_CONSTRAINTS, | ||
| ) | ||
|
|
||
| genrule( | ||
| name = "gen-crdb-test-off", | ||
| srcs = ["crdb_test_off.go"], | ||
| outs = ["gen-crdb_test_off.go"], | ||
| cmd = REMOVE_GO_BUILD_CONSTRAINTS, | ||
| ) | ||
|
|
||
| go_test( | ||
| name = "pebbleiter_test", | ||
| srcs = ["crdb_test_test.go"], | ||
| args = ["-test.timeout=295s"], | ||
| embed = [":pebbleiter"], # keep | ||
| deps = ["@com_github_stretchr_testify//require"], | ||
| ) | ||
|
|
||
| get_x_data(name = "get_x_data") |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| // Copyright 2022 The Cockroach Authors. | ||
| // | ||
| // Use of this software is governed by the Business Source License | ||
| // included in the file licenses/BSL.txt. | ||
| // | ||
| // As of the Change Date specified in that file, in accordance with | ||
| // the Business Source License, use of this software will be governed | ||
| // by the Apache License, Version 2.0, included in the file | ||
| // licenses/APL.txt. | ||
|
|
||
| //go:build !crdb_test || crdb_test_off | ||
| // +build !crdb_test crdb_test_off | ||
|
|
||
| package pebbleiter | ||
|
|
||
| import "github.com/cockroachdb/pebble" | ||
|
|
||
| // Iterator redefines *pebble.Iterator. | ||
| type Iterator = *pebble.Iterator | ||
|
|
||
| // MaybeWrap returns the provided Iterator, unmodified. | ||
| func MaybeWrap(iter *pebble.Iterator) Iterator { | ||
| return iter | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| // Copyright 2022 The Cockroach Authors. | ||
| // | ||
| // Use of this software is governed by the Business Source License | ||
| // included in the file licenses/BSL.txt. | ||
| // | ||
| // As of the Change Date specified in that file, in accordance with | ||
| // the Business Source License, use of this software will be governed | ||
| // by the Apache License, Version 2.0, included in the file | ||
| // licenses/APL.txt. | ||
|
|
||
| //go:build crdb_test && !crdb_test_off | ||
| // +build crdb_test,!crdb_test_off | ||
|
|
||
| package pebbleiter | ||
|
|
||
| import "github.com/cockroachdb/pebble" | ||
|
|
||
| // Iterator wraps the *pebble.Iterator in crdb_test builds with an AssertionIter | ||
| // that detects when Close is called on the iterator twice. Double closes are | ||
| // problematic because they can result in an iterator being added to a sync pool | ||
| // twice, allowing concurrent use of the same iterator struct. | ||
| type Iterator = *AssertionIter | ||
|
|
||
| // MaybeWrap returns the provided Pebble iterator, wrapped with double close | ||
| // detection. | ||
| func MaybeWrap(iter *pebble.Iterator) Iterator { | ||
| return &AssertionIter{Iterator: iter} | ||
| } | ||
|
|
||
| // AssertionIter wraps a *pebble.Iterator with assertion checking. | ||
| type AssertionIter struct { | ||
| *pebble.Iterator | ||
| closed bool | ||
| } | ||
|
|
||
| func (i *AssertionIter) Clone(cloneOpts pebble.CloneOptions) (Iterator, error) { | ||
| iter, err := i.Iterator.Clone(cloneOpts) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return MaybeWrap(iter), nil | ||
| } | ||
|
|
||
| func (i *AssertionIter) Close() error { | ||
| if i.closed { | ||
| panic("pebble.Iterator already closed") | ||
| } | ||
| i.closed = true | ||
| return i.Iterator.Close() | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| // Copyright 2022 The Cockroach Authors. | ||
| // | ||
| // Use of this software is governed by the Business Source License | ||
| // included in the file licenses/BSL.txt. | ||
| // | ||
| // As of the Change Date specified in that file, in accordance with | ||
| // the Business Source License, use of this software will be governed | ||
| // by the Apache License, Version 2.0, included in the file | ||
| // licenses/APL.txt. | ||
|
|
||
| package pebbleiter | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestPebbleIterWrapped(t *testing.T) { | ||
| // Sanity-check: make sure CrdbTestBuild is set. This should be true for | ||
| // any test. | ||
| require.NotNil(t, MaybeWrap(nil)) | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| // Copyright 2022 The Cockroach Authors. | ||
| // | ||
| // Use of this software is governed by the Business Source License | ||
| // included in the file licenses/BSL.txt. | ||
| // | ||
| // As of the Change Date specified in that file, in accordance with | ||
| // the Business Source License, use of this software will be governed | ||
| // by the Apache License, Version 2.0, included in the file | ||
| // licenses/APL.txt. | ||
|
|
||
| // Package pebbleiter exposes a type that selectively wraps a Pebble Iterator | ||
| // only in crdb_test builds. This wrapper type performs assertions in test-only | ||
| // builds. In non-test-only builds, pebbleiter exposees a direct type alias of | ||
| // the pebble.Iterator. | ||
| package pebbleiter |