Skip to content

fix: Prevent race between provisionerd connect and close #6206

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
Merged
merged 3 commits into from
Feb 14, 2023
Merged

fix: Prevent race between provisionerd connect and close #6206

merged 3 commits into from
Feb 14, 2023

Conversation

mafredri
Copy link
Member

I'm hoping to have finally narrowed this down:

goleak: Errors on successful test run: found unexpected goroutines:
[Goroutine 10 in state chan receive, with storj.io/drpc/drpcserver.(*Server).Serve.func1 on top of the stack:
goroutine 10 [chan receive]:
storj.io/drpc/drpcserver.(*Server).Serve.func1({0xc3eea0?, 0xc0004641b0?})
	/home/mafredri/.local/go/pkg/mod/storj.io/drpc@v0.0.33-0.20220622181519-9206537a4db7/drpcserver/server.go:80 +0x45
storj.io/drpc/drpcctx.(*Tracker).track(0xc0004641b0, 0x0?)
	/home/mafredri/.local/go/pkg/mod/storj.io/drpc@v0.0.33-0.20220622181519-9206537a4db7/drpcctx/transport.go:52 +0x2b
created by storj.io/drpc/drpcctx.(*Tracker).Run
	/home/mafredri/.local/go/pkg/mod/storj.io/drpc@v0.0.33-0.20220622181519-9206537a4db7/drpcctx/transport.go:47 +0x8d

 Goroutine 9 in state chan receive, with github.com/valyala/fasthttp/fasthttputil.(*InmemoryListener).Accept on top of the stack:
goroutine 9 [chan receive]:
github.com/valyala/fasthttp/fasthttputil.(*InmemoryListener).Accept(0xc0004641b0?)
	/home/mafredri/.local/go/pkg/mod/github.com/valyala/fasthttp@v1.44.0/fasthttputil/inmemory_listener.go:50 +0x35
storj.io/drpc/drpcserver.(*Server).Serve(0xc0004aa000, {0xc3e810, 0xc00015a000}, {0xc3d230?, 0xc000122000})
	/home/mafredri/.local/go/pkg/mod/storj.io/drpc@v0.0.33-0.20220622181519-9206537a4db7/drpcserver/server.go:85 +0x1bb
github.com/coder/coder/provisionerd_test.createProvisionerDaemonClient.func3()
	/home/mafredri/src/coder/coder/provisionerd/provisionerd_test.go:1096 +0x6c
created by github.com/coder/coder/provisionerd_test.createProvisionerDaemonClient
	/home/mafredri/src/coder/coder/provisionerd/provisionerd_test.go:1094 +0x485
]

By refactoring the tests, I found this:

panic: Fail in goroutine after TestProvisionerd/InstantClose has completed

goroutine 130 [running]:
testing.(*common).Fail(0xc00050cea0)
	/usr/local/go/src/testing/testing.go:933 +0x1a8
testing.(*common).Error(0xc00050cea0, {0xc0007a1c80, 0x1, 0x1})
	/usr/local/go/src/testing/testing.go:1043 +0x8f
github.com/coder/coder/provisionerd_test.createProvisionerDaemonClient(0xc00050cea0, 0xc0001b6120, {0x0, 0x0, 0x0, 0x0, 0x0})
	/home/mafredri/src/coder/coder/provisionerd/provisionerd_test.go:1173 +0x92d
github.com/coder/coder/provisionerd_test.TestProvisionerd.func2.2({0xc00056ce38?, 0x14a0540?})
	/home/mafredri/src/coder/coder/provisionerd/provisionerd_test.go:63 +0x7f
github.com/coder/coder/provisionerd.(*Server).connect(0xc000792000, {0x14a0540?, 0xc00078c000})
	/home/mafredri/src/coder/coder/provisionerd/provisionerd.go:169 +0x209
created by github.com/coder/coder/provisionerd.New
	/home/mafredri/src/coder/coder/provisionerd/provisionerd.go:109 +0x88e
FAIL	github.com/coder/coder/provisionerd	0.048s
FAIL

This lead to the fix in connect where we might end up with a successfull connection after close.

  • fix: Prevent race between provisionerd connect and close
  • test: Add detection for provisioner creation after test completion

@mafredri mafredri self-assigned this Feb 14, 2023
@mafredri mafredri requested a review from kylecarbs February 14, 2023 15:58
kylecarbs
kylecarbs approved these changes Feb 14, 2023
View reviewed changes
Copy link
Member

@kylecarbs kylecarbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥳🥳🥳🥳🥳🥳

@mafredri mafredri enabled auto-merge (squash) February 14, 2023 16:06
@mafredri mafredri merged commit 860e282 into main Feb 14, 2023
@mafredri mafredri deleted the mafredri/test-try-to-capture-provisionerd-test-issue branch February 14, 2023 16:37
@github-actions github-actions bot locked and limited conversation to collaborators Feb 14, 2023
@mafredri mafredri added release/experimental These changes are feature-flagged, they may change or be removed in future releases and removed release/experimental These changes are feature-flagged, they may change or be removed in future releases labels Feb 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kylecarbs kylecarbs kylecarbs approved these changes

Assignees

@mafredri mafredri

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mafredri @kylecarbs