After we started using the module on our page, we noticed some error messages appearing from time to time in our error logs:
Error: Call to a member function delete() on null in Drupal\recently_read\RecentlyReadService->deleteRecords() (line 150 of ...modules/contrib/recently_read/src/RecentlyReadService.php).
We investigated the code and tried to reproduce the issue, and we came to the conclusion that this is a (rare) concurrency issue with the records deletion. For example, if you use count-based record deletion (e.g., set to 3), and then open more than 5 pages within a very short amount of time, it can happen that two requests (from the same user/session) retrieve the same existing records in this code snippet in the RecentlyReadService, before either of them deletes the existing records:
// Delete records if there is a limit.
$userRecords = $this->getRecords($user->id());
if ($maxRecords && count($userRecords) > $maxRecords) {
$records = array_slice($userRecords, $maxRecords, count($userRecords));
$this->deleteRecords($records);
}
If that happens, in both concurrent executions, the code will try to delete the records, and one of them will then fail because the record to delete doesn't exist anymore.
As a result, the user will be faced with an "Unexpected error" message, rendering the site unusable. While this can be easily fixed by reloading the page, it still strongly negatively affects the user experience (although it happens only rarely, luckily).
1. Install the module, enable tracking for nodes and configure "Records delete options" to "Count-based" and set it to "3".
2. Visit the page as any user.
3. Open 5 nodes within a very short amount of time.
4. If you're "unlucky", an unexpected error can occur due to conncurrency, as described above.
Modify the deleteRecords method in a way that it checks whether a record still exists before trying to delete it:
public function deleteRecords(array $records): void {
foreach ($records as $rid) {
// Delete data.
$recently_read = $this->recentlyReadStorage->load($rid);
if ($recently_read instanceof RecentlyRead) {
$recently_read->delete();
}
}
}
- Create MR
- Review
Active
1.4
Code
Hi,
I've tried to replicate the issue you've been describing and unfortunately I haven't been able too. However I agree that this should get merged in for the next release because implementing that additional check before deleting entries is good practice anyway.
Marking this as RTBC.