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

The deadlock problem occurs when updating elements in the OnRemove method #129

Closed
weiwei02 opened this issue May 20, 2019 · 9 comments
Closed

Comments

@weiwei02
Copy link

Many times I have a requirement that when an element expires and the OnRemove method is called, I want to retrieve the latest value for that key from the remote service and put it back into bigcache. But the existing shard clean method locks the shard, and the set method of bigcache locks the same shard method. Therefore, the call to the set method in the OnRemove function satisfies the necessary condition of deadlock and fails to meet the requirement of automatic update when the cache expires.

@cristaloleg
Copy link
Collaborator

What about passing an expired object to a chan and put item from chan back to the cache?

@weiwei02
Copy link
Author

Yes, the idea is to fix the problem of automatic updates when they expire. However, the time to call the remote service is unpredictable during automatic update of expired elements, so it may be more appropriate to turn on multiple goroutines in parallel to perform this expired automatic update task. I think bigcache is a good framework to support this.

@cristaloleg
Copy link
Collaborator

remote service

I don't get it, where do we have a remote service in such case?

bigcache is a good framework

I doubt that we can name bigcache a framework, it's more like a library 😉

@weiwei02
Copy link
Author

For example, when my local program needs to store the latest application configuration information (for example, the product snapshot of jd.com and the QPS standard for data query is 4 million /s), the configuration information is persisted and stored in mysql, and the data in mysql can be updated by many other processes. In this case, I want to use bigcache to improve the query performance of the application, but I also want to ensure that the data I find is "as up-to-date as possible", so I set the data in bigcache and mysql to be synchronized every 1 second.

@weiwei02
Copy link
Author

@cristaloleg I haven't contributed code to github before fork a new branch of code to try to solve this problem. The idea is that developers configure the number of goroutines to be turned on in bigcache based on the performance of their remote service. Where do you think this method can be improved?

@cristaloleg
Copy link
Collaborator

I will review it with @mxplusb soon (we're talking about this branch master...weiwei02:expire-func)

@weiwei02
Copy link
Author

@cristaloleg Thank you. If necessary, I will remove the ants library and write the task scheduling code that will reuse goroutine.

@siennathesane
Copy link
Collaborator

So I have a couple concerns with the implementation details that I think can be worked through.

Let's not use a goroutine pool. Generally goroutine pools aren't aligned well with idiomatic golang design patterns, and it adds a layer of abstraction not well suited to go. Goroutines are incredibly cheap and the worker pattern in this case doesn't add enough of a performance benefit to outweigh the maintenance cost. If it was a core path, like the GET/SET/DELETE paths, then I might be able to be persuaded, but if the use case is just for handling callbacks, let's look at callbacks, something like OnRemoveWithReason. We took in OnRemoveWithReason specifically because it left the specific action of what to do when an asynchronous key event fired to the downstream implementor, and I think that's still the right path.

Reading through your issue, where you want to retrieve the latest key value and then return it to the shard, is that not what is happening now? At no point should you be interfacing with the shard because it's not designed to be safe for access, that's what bigcache.{Get,Set,Delete} are for. OnRemoveWithReason should give you the key, the payload (values), and the reason it was deleted.

  • Is there a reason OnRemoveWithReason isn't enough to solve your problem?
  • Would an interface be more appropriate for OnRemoveWithReason, where you can define significantly more than just the single function?

@weiwei02
Copy link
Author

@

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

No branches or pull requests

3 participants