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
Fixes closing of Cache and Policy to be concurrent safe #286
base: master
Are you sure you want to change the base?
Conversation
|
One open question: Should we still try to close out the |
61af594
to
074370d
Compare
I can't see the TeamCity build results (no access), but |
Thanks for the PR @josephschorr. CI is failing with a data race.
Try running the tests with the
|
D'oh... I forgot the race detector doesn't run automatically. Will address now. |
@danielmai So looking more deeply, the race is more or less the "expected behavior" on that variable, which is why I keep a local copy. I can easily fix this by adding a mutex guard around accessing it, but that does mean that there will be some slight slowdown on all the function calls. Thoughts? Edit: Decided to add it for safety |
Before this change, calling Close on the cache while a Set call is in progress could occasionally result in a panic when the Set method tried to write to the (now closed) `setBuf` channel. This commit changes Cache and Policy to keep references to the written-to channels in an `internals` struct, which is nil-ed out on close and checked in each call site, ensuring that nothing is attempting to write to a closed channel.
074370d
to
169a43a
Compare
I'll defer to @manishrjain to chime in for this PR. |
… the parser Requires dgraph-io/ristretto#286 before it can be rebased and merged
@manishrjain Thoughts on the use of a lock vs, say, a compare and swap? In terms of timing, we're trying to decide whether to have SpiceDB make use of my branch or wait for this to merge and release, so guidance in that area would also be appreciated. |
Hey @josephschorr , for now, my recommendation would be to use your fork. Meanwhile, I'll try to find some time to review this PR and understand it's performance implications. |
… the parser Requires dgraph-io/ristretto#286 before it can be rebased and merged Signed-off-by: Joseph Schorr <josephschorr@users.noreply.github.com>
…etto#286 is merged Signed-off-by: Joseph Schorr <josephschorr@users.noreply.github.com>
…etto#286 is merged Signed-off-by: Joseph Schorr <josephschorr@users.noreply.github.com>
… the parser Depends upon dgraph-io/ristretto#286 Signed-off-by: Joseph Schorr <josephschorr@users.noreply.github.com>
…etto#286 is merged Signed-off-by: Joseph Schorr <josephschorr@users.noreply.github.com>
Any updates regarding this PR ? |
Friendly ping @josephschorr @manishrjain |
I'm no longer managing this repo. I have my own fork of ristretto that I manage and use. |
@josephschorr would you please accept the CLA and resolve the conflict. @joshua-goldstein, @pandeyshubham25, @matthewmcneely could one of you review the code and help with resolving the conflict. Also, let's make sure that the version is updated appropriately. |
@akon-dey I'd be happy to rebase my changes, but be aware we've moved to the github.com/outcaste-io/ristretto fork, which fixed the issue as well. This is partially why we stopped asking for updates on our fork containing this branch. I'll see if I can get this rebased this evening |
@josephschorr I was wondering what's the reasoning for using the fork, is not like this repo was abandoned? |
@gaby This PR sat for quite a while, so we used our own fork during that time. Once github.com/outcaste-io/ristretto became available, we tested it for the fix, found it worked, and so moved to it rather than relying on a custom fork that would not get updates otherwise. |
Makes sense, thanks for the feedback! |
Before this change, calling Close on the cache while a Set call is in progress could occasionally result in a panic when the Set method tried to write to the (now closed)
setBuf
channel. This commit changes Cache and Policy to keep references to the written-to channels in aninternals
struct, which is nil-ed out on close and checked in each call site, ensuring that nothing is attempting to write to a closed channel.This change is