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.

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

Use buffered channels for lookup results #259

Merged
merged 1 commit into from
Nov 5, 2021

Conversation

ecordell
Copy link
Contributor

@ecordell ecordell commented Nov 4, 2021

This prevents leaf goroutines from getting stuck on write to a channel nothing is reading from

(I haven't convinced myself that there are no other goroutine leaks in the lookup dispatch, but this is definitely a source of leaks)

@github-actions github-actions bot added area/api v1 Affects the v1 API area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) labels Nov 4, 2021
this prevents leaf goroutines from getting stuck on write to a closed
channel
@@ -378,6 +379,7 @@ func TestLookupResources(t *testing.T) {
t.Run(fmt.Sprintf("%s::%s from %s:%s#%s", tc.objectType, tc.permission, tc.subject.Object.ObjectType, tc.subject.Object.ObjectId, tc.subject.OptionalRelation), func(t *testing.T) {
require := require.New(t)
client, stop, revision := newPermissionsServicer(require, delta, memdb.DisableGC, 0)
defer goleak.VerifyNone(t, goleak.IgnoreCurrent())
Copy link
Contributor Author

@ecordell ecordell Nov 5, 2021

Choose a reason for hiding this comment

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

it looks weird to have this before defer stop(), but that lets IgnoreCurrent capture the running server's goroutines and check for leaks after stop()

Copy link
Member

@josephschorr josephschorr left a comment

Choose a reason for hiding this comment

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

LGTM

@ecordell ecordell merged commit 964b7a7 into authzed:main Nov 5, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Nov 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/api v1 Affects the v1 API area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants