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.

Sign up for GitHub

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

Jump to bottom

streamclient: panic in TestSinklessReplicationClient #68168

Closed
knz opened this issue Jul 28, 2021 · 1 comment · Fixed by #68189
Closed

streamclient: panic in TestSinklessReplicationClient #68168

knz opened this issue Jul 28, 2021 · 1 comment · Fixed by #68189
Assignees
@pbardea
Labels
branch-master Failures and bugs on the master branch. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. C-test-failure Broken test (automatically or manually discovered). T-disaster-recovery

Comments

@knz
Copy link
Contributor

knz commented Jul 28, 2021 •
edited
Loading

https://teamcity.cockroachdb.com/buildConfiguration/Cockroach_UnitTests/3234996?expandedTest=build%3A%28id%3A3234990%29%2Cid%3A130957

panic: close of closed channel

goroutine 2276 [running]:
github.com/cockroachdb/cockroach/pkg/ccl/streamingccl/streamclient.(*sinklessReplicationClient).ConsumePartition.func1(0xc0003d6540, 0xc0022c47e0, 0xc00136c9c0, 0xc0028c1200, 0x58448f0, 0xc00218ce00)
	/go/src/github.com/cockroachdb/cockroach/pkg/ccl/streamingccl/streamclient/cockroach_sinkless_replication_client.go:128 +0x6f5
created by github.com/cockroachdb/cockroach/pkg/ccl/streamingccl/streamclient.(*sinklessReplicationClient).ConsumePartition
	/go/src/github.com/cockroachdb/cockroach/pkg/ccl/streamingccl/streamclient/cockroach_sinkless_replication_client.go:88 +0x3f0

goroutine 1 [chan receive]:
testing.(*T).Run(0xc000489680, 0x4468a5f, 0x1d, 0x4a73ca0, 0x4b31a6)
	/usr/local/go/src/testing/testing.go:1239 +0x2da
testing.runTests.func1(0xc000489500)
	/usr/local/go/src/testing/testing.go:1511 +0x78
testing.tRunner(0xc000489500, 0xc000c7fd60)
	/usr/local/go/src/testing/testing.go:1193 +0xef
testing.runTests(0xc0007da060, 0x75cb490, 0x1, 0x1, 0xc03869a2bab3f627, 0x274afbd7e58, 0x7931660, 0xc000978740)
	/usr/local/go/src/testing/testing.go:1509 +0x2fe
testing.(*M).Run(0xc000bc7d00, 0x0)
	/usr/local/go/src/testing/testing.go:1417 +0x1eb
github.com/cockroachdb/cockroach/pkg/ccl/streamingccl/streamclient.TestMain(0xc000bc7d00)
	/go/src/github.com/cockroachdb/cockroach/pkg/ccl/streamingccl/streamclient/main_test.go:31 +0xde
main.main()
	_testmain.go:47 +0x165
@knz knz added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. C-test-failure Broken test (automatically or manually discovered). branch-master Failures and bugs on the master branch. labels Jul 28, 2021
@knz knz mentioned this issue Jul 28, 2021
Merged
@knz
Copy link
Contributor Author

knz commented Jul 28, 2021

related to #67592
cc @adityamaru

@pbardea pbardea self-assigned this Jul 28, 2021
adityamaru added a commit to adityamaru/cockroach that referenced this issue Jul 28, 2021
@adityamaru
streamingccl: fix double close in test feed source
aca069d 
Previously, the close implementation of channelFeedSource
closed the `eventCh` as a means of shutting down the client.
This seems wrong since the client is the "owner" of this
channel and should be the only one responsible for closing
it. In cases where the method `consumeUntil` hit an error
(max wait timeout in our case) we would close the eventCh
as part of calling channelFeedSource's Close(), and we would
attempt a double close in the sinkless client itself.

This change switches to cancelling the sinkless client's
context to indicate to the client that it should shutdown.

I was not able to reproduce this error (5000 stress runs),
but I bumped the timeout from 10 seconds to 20 seconds
based on stack traces in the failed builds.

Fixes: cockroachdb#68168
Fixes: cockroachdb#67592

Release note: None
@adityamaru adityamaru mentioned this issue Jul 28, 2021
Merged
craig bot pushed a commit that referenced this issue Jul 28, 2021
@pawalt @rickystewart @adityamaru
Merge #66735 #68017 #68078 #68189
dbd4e53 
66735: sql: add datadriven tests for multiregion tables r=arulajmani a=pawalt

Previously, we only had datadriven tests for GLOBAL tables.
This patch adds datadriven tests for REGIONAL BY TABLE tables.

Release note: None

68017: bazel: bazelfy logging documentation r=rail a=rickystewart

In doing so, I also noticed a couple generated `.go` files that weren't
yet bazelfied, so I took care of them too.

Closes #65812.

Release note: None

68078: bazel: add build target for generated bnf's r=rail a=rickystewart

Closes #65815.

Release note: None

68189: streamingccl: fix double close in test feed source r=miretskiy a=adityamaru

Previously, the close implementation of channelFeedSource
closed the `eventCh` as a means of shutting down the client.
This seems wrong since the client is the "owner" of this
channel and should be the only one responsible for closing
it. In cases where the method `consumeUntil` hit an error
(max wait timeout in our case) we would close the eventCh
as part of calling channelFeedSource's Close(), and we would
attempt a double close in the sinkless client itself.

This change switches to cancelling the sinkless client's
context to indicate to the client that it should shutdown.

I was not able to reproduce this error (5000 stress runs),
but I bumped the timeout from 10 seconds to 20 seconds
based on stack traces in the failed builds.

Fixes: #68168
Fixes: #67592

Release note: None

Co-authored-by: Peyton Walters <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
Co-authored-by: Aditya Maru <[email protected]>
@craig craig bot closed this as completed in 080f904 Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@pbardea pbardea

Labels
branch-master Failures and bugs on the master branch. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. C-test-failure Broken test (automatically or manually discovered). T-disaster-recovery
Projects
No open projects
Disaster Recovery Backlog
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

streamingccl: fix double close in test feed source adityamaru/cockroach
2 participants
@pbardea @knz