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

bug: fix a logical (locking) error in the Loader.Load method #113

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

svanharmelen
Copy link

I've noticed that this package isn't updated for quite a while now, but I hope this PR can be merged and warrants a new release. The reason for opening this PR is because we encountered some errors where a request wouldn't get a result.

It took some debugging and searching but in the end we found out what happened. I tried to write a test for it, but as it is very time sensitive (a bunch of things need to happen at more or less the exact same time) I was not able to come up with a way to write such a test. So hopefully describing the issue here is enough to explain both the problem and the fix...

The problem is related to the locking logic. Currently the Loader.Load method first locks the cacheLock to check and, if needed, insert an new request and then unlocks the cacheLock again.

Right after that it lock the batchLock in order to, if needed, create a new batcher, then send the new request to the batcher and finally check if batchCap is reached in which case it does two things:

  1. it closes the batchers' input channel which causes the batcher to "execute" the batch
  2. it resets the cache if the dataloader is configured with clearCacheOnBatch

Note that these two things will also happen when a sleeping batcher reaches its wait timeout!

Now if a new request comes in, gets passed the cacheLock and then needs to wait for the batchLock, it might be that the logic that currently holds the batchLock will clear the cache and reset the batcher so a new batcher will be created. In that case the request that was inserted in the "old" cache, will now be send to a "new" batcher as soon as the batchLock is released.

If in this specific situation the same request is send again (so before the "new" batcher is executed) it will be seen as a new request to the cache, so it will be inserted in the cache and send to the batcher again which will break the guarantee that each request send to the batcher will be unique. This in turn causes different types of issues depending on the logic you've written in your loaders.

Hope this all makes sense, please let me know if you need anything else from me in order to get this merged and released.

Thanks!

@svanharmelen
Copy link
Author

Do you need anything from me in order to get this one merged/released?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant