leaktest: print non-closed stoppers #51413
Conversation
|
This change is |
There was a problem hiding this comment.
LGTM
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @tbg)
pkg/testutils/localtestcluster/local_test_cluster.go, line 115 at r1 (raw file):
ltc.tester = t if ltc.Stopper == nil {
can ltc.Stopper be non-nil? Does someone assign it manually? I don't see anyone doing it. How about passing it in to this Start()?
pkg/util/stop/stopper.go, line 66 at r1 (raw file):
type stopperWithStack struct { s *Stopper stack []byte
comment this pls or give it a more suggestive name. Can it be a string?
|
PTAL @andreimatei I did not add the task yet since I am running out of time (back only on Tuesday). Feel free to add the task separately in another PR. However I think this PR alone will also already help since we're not going to have one-off stoppers active longer than the test servers' any more. |
There was a problem hiding this comment.
LGTM
Reviewable status:
In cockroachdb#51363 we saw use-after-close of an engine, which I tracked down to `LocalTestCluster` using more than one stopper (only one of which was actually stopped). This is likely a larger class of problem, so I added a facility to ensure that no stoppers are currently active, which is invoked from leaktest. However, due to the large amount of work that is necessary to sanitize the whole testing codebase, this is for now only advisory (i.e. logs, does not error). Nevertheless, this will be useful in tests in which a use-after-stop actually causes problems. For the LocalTestCluster, I made sure that the warning is no longer printed, which mainly entailed not handing a different stopper to the kv DB than that used for the main server. This explains the failures we have been seeing, which always involved a KV client request. Closes cockroachdb#51363. Release note: None
andreimatei
force-pushed
the
leaktest-stoppers
branch
from
3f9b393 to
3266fd9
Compare
DefaultDBContext previously created an internal stopper, but nobody was ever stopping it. Nothing interesting happens in this change. Release note: None
andreimatei
force-pushed
the
leaktest-stoppers
branch
from
3266fd9 to
8ba28dc
Compare
There was a problem hiding this comment.
bors r+
Reviewable status:
Build failed (retrying...) |
There was a problem hiding this comment.
bors r+
Reviewable status:
Build succeeded |
This looks harmless because the first stopper was never actually used, but it was caught by cockroachdb#51413.
51807: testcluster: don't create two stoppers in StartTestCluster r=nvanbenschoten a=nvanbenschoten This looks harmless because the first stopper was never actually used, but it was caught by #51413. 51832: dump: fix dump-backwards-compatibility remote roachtest r=pbardea a=pbardea This commit changes the execution of the cockroach binary from `cockroach` to `./cockroach`. This follows how other CLI roachtests run the binary. Before this commit, this test would fail when run on a GCE roachprod cluster, it now passes. Release note: None Co-authored-by: Nathan VanBenschoten <[email protected]> Co-authored-by: Paul Bardea <[email protected]>
In #51363 we saw
use-after-close of an engine, which I tracked down to
LocalTestClusterusing more than one stopper (only one of which was actually stopped).
This is likely a larger class of problem, so I added a facility to
ensure that no stoppers are currently active, which is invoked from
leaktest. However, due to the large amount of work that is necessary
to sanitize the whole testing codebase, this is for now only advisory
(i.e. logs, does not error). Nevertheless, this will be useful in tests
in which a use-after-stop actually causes problems.
For the LocalTestCluster, I made sure that the warning is no longer
printed, which mainly entailed not handing a different stopper to the kv
DB than that used for the main server. This explains the failures we
have been seeing, which always involved a KV client request.
Closes #51363.
Release note: None