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

fix: prevent race condition when marshalling cached operations #96

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+5 −2
Open

fix: prevent race condition when marshalling cached operations #96

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -575,8 +575,11 @@ func marshal(q interface{}, wrapper string, fields Fields) (string, error) { //n

// Check to see if this type has already been built.
if cachedOperation, hit := cache.Load(rt); hit {
// Cache hit, use the tree that was already built.
operation = cachedOperation.(*field)
// Cache hit, copy by value the tree that was already built.
// A copy is needed because the top level declaration name (primitive type)
// of the operation is re-assigned.
opReplica := *cachedOperation.(*field)
operation = &opReplica
Copy link

Choose a reason for hiding this comment

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

I am afraid we need deeper refactoring.
Current "buggy" implementation modifies operation data (Decl) stored in cache after being retrieved from cache. So supposedly next time it is retrieved from cache it has new pieces.

This implementation modifies data on a copy of cached entry so that change is not visible to the next caller that hits the cache.

And maybe it is fine - the Decl part should be custom to each cycle. In that case we should probably removed it from cache entry and have it as local var in this function.

But if it is integral part of cache entry w'll probably need larger sync scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. The updated field in Declaration is definitely not integral part of the cache entry. Therefore, next retrievals don't need to be aware of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Y'all are right on the money with your analysis as far as I can tell. I'll approve this in the interim though

} else {
// Not in cache, need to build by walking through the type and then store it in the
// cache for later use.
Expand Down