Replies: 15 comments 4 replies
-
|
I'd welcome a PR to add a failing test to reproduce this error. |
Beta Was this translation helpful? Give feedback.
-
|
I can do that, but as I said this issue happens rarely, so the test would be: And I think that's not a good test, since the fix should be somewhere in Middlewares/CacheResponse.php. |
Beta Was this translation helpful? Give feedback.
-
|
I'm also having this issues and as I have hundreds of thousands of requests per day, I have this quite often. |
Beta Was this translation helpful? Give feedback.
-
|
Just for you guys have an idea, I had this 18 times in the last 7 days: |
Beta Was this translation helpful? Give feedback.
-
|
This issue still exists. When load-testing my app, it gives this error 5/250 times. |
Beta Was this translation helpful? Give feedback.
-
|
I am not sure how you would write a test for this without adding a parallel execution library. The problem is that there is a race condition that happens when two requests deal with the same cache data and one clears it while the other is in the process of verifying and then reading it. Let's say we have two requests, Request A and Request B. Request A comes in and winds up clearing the cache. Request B is requesting the cached data. Request A -> Insert // In CacheResponse handle
// There likely exists a race condition between hasBeenCached and getCachedResponseFor
// Code from v6.6.9
if ($this->responseCache->enabled($request)) {
if ($this->responseCache->hasBeenCached($request, $tags)) {
event(new ResponseCacheHit($request));
// Request A (likely) nukes cache at this point
$response = $this->responseCache->getCachedResponseFor($request, $tags);
$this->getReplacers()->each(function (Replacer $replacer) use ($response) {
$replacer->replaceInCachedResponse($response);
});
return $response;
}
}This is my guess anyway. After adding a cache clear to a specific endpoint on a very high traffic service, this error started appearing in large numbers. This service has a < 80ms response time with a few thousand users a minute, so I reckon we are hitting this edge case a lot more than is typical. My first thought, if I am correct about the race condition, is to remove the call to hasBeenCached and just rely on checking the result from getCachedResponseFor. edit: Maybe a better solution is to catch exceptions for that call, log, and then treat the request as uncached instead. |
Beta Was this translation helpful? Give feedback.
-
This is correct, we have a lot of project using this awesome library, and only one project occur this, and that project is a huge request, notice this when one of model resource is being updated and clearing cache, it will throw consecutive error of |
Beta Was this translation helpful? Give feedback.
-
|
@swichers can you try PR of your idea? |
Beta Was this translation helpful? Give feedback.
-
|
This is a huge issue for a projects that are big enough for this "race condition". |
Beta Was this translation helpful? Give feedback.
-
|
Upgrade to last version, it has been fixed in #383 |
Beta Was this translation helpful? Give feedback.
-
|
I'm not sure if #383 has fixed the issue. The exception won't be raised anymore, but the |
Beta Was this translation helpful? Give feedback.
-
|
We can solve it by overriding handle method of CacheResoponse middleware. Do nothing for new exception and cache again response. |
Beta Was this translation helpful? Give feedback.
-
|
Like @lisotton said, that PR does not fix the problem being reported on this issue. It changes the nature of the failure. The race condition still exists, it's just been masked. Replacing the middleware with your own is also not an ideal workaround because it duplicates the code into your own codebase. This makes you responsible for integrating future bugfixes or updates. If the module gets refactored your copied code may fail to continue to work. Not very DRY either. The most likely proper fix for this is to rely on the response from Another option would be to refactor the method so several of its responsibilities are broken out into separate methods (more SOLID approach). At that point a custom middleware in your own codebase would not be duplicating a lot of third party package logic. |
Beta Was this translation helpful? Give feedback.
-
|
Im agree with you about refactoring the method, it can help to solve the problem more efficient. |
Beta Was this translation helpful? Give feedback.
-
|
The A key lookup vs a failed get should be exactly the same speed. If the result of your successful key lookup is always you retrieving the full item, you should just attempt to retrieve the full item to begin with. But we don't have to take my word for it, let's look at the actual code: hasBeenCached: getCachedResponseFor: They ultimately pass off logic to Let's see how core implements the https://github.com/illuminate/cache/blob/master/Repository.php#L68= public function has($key): bool
{
return ! is_null($this->get($key));
}So we can see the current code tries to retrieve the cached value twice when cache exists. |
Beta Was this translation helpful? Give feedback.
-
|
Yes, you are right. |
Beta Was this translation helpful? Give feedback.
-
|
I'd like to +1 this issue, it's rare but annoying and difficult to explain to the unlucky few who might hit it when using the API. Could we not just do (with some editing to Am I missing something? Or in the interim, could we have it return an uncached response instead of just throwing the exception to the user? |
Beta Was this translation helpful? Give feedback.
-
|
For this race condition problem. Can we refer to the nginx proxy cache lock mechanism for the same design ? |
Beta Was this translation helpful? Give feedback.
-
|
I have a pull request #434 which resolves this issue. I left in the technically unnecessary call to |
Beta Was this translation helpful? Give feedback.
-
Trying to reopen #137.
For some reason after the cache repository confirms the response
hasBeenCached(),trying to
getCachedResponseFor()fails asunserialize()is gettingnullinstead of astringfrom the cache.This happens very rarely.
Exception:
Beta Was this translation helpful? Give feedback.
All reactions