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

Wrong success check in cache FileBackend.writeCacheFile() #2677

Closed
kdambekalns opened this issue Jan 20, 2022 · 1 comment
Closed

Wrong success check in cache FileBackend.writeCacheFile() #2677

kdambekalns opened this issue Jan 20, 2022 · 1 comment

Comments

@kdambekalns
Copy link
Member

Description

When the same session is used concurrently, sometimes the session seemingly ends.

The log then shows an exception about unserialize() not being able to process the value retrieved from the cache backend in

return ($this->useIgBinary === true) ? igbinary_unserialize($rawResult) : unserialize($rawResult);

The reason in this case is an incomplete data file when using the FileBackend (other backends are probably not affected) which is weird, because our cache writes should be atomic.

Steps to Reproduce

  1. Have concurrent requests using a session
  2. You will eventually observe Possible race condition in VariableFrontend.get() #2672
  3. You conclude this should never happen due to writes being atomic

Expected behavior

A cache file is either written or not.

Actual behavior

Incomplete cache files are written.

Reason

Maybe this line is wrong?

$result = fwrite($file, $data) === false ?: true;

$result is always true afterwards, which certainly is not intended… that has been changed in e80c5a9#diff-81c872d2429d48a1767632c0255f66f1281ec7a1a339b2f715b9af72322409f4L541-R541

Affected Versions

Flow: 6.3+

@kdambekalns
Copy link
Member Author

kdambekalns commented Jan 20, 2022

Further research on atomicity here:

LOCK_EX will not prevent reading the file unless you also explicitly acquire a read lock (shared locked), so that other pcoesses may very well read the file while it is written, leading to incomplete data.

But in readCacheFile() we do

if (flock($file, LOCK_SH) !== false) {
    if ($offset !== null) {
        fseek($file, $offset);
    }
    $data = fread($file, $maxlength ?? (filesize($cacheEntryPathAndFilename) - (int)$offset));
    flock($file, LOCK_UN);
}

so indeed our cache file operations should be atomic.

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

No branches or pull requests

1 participant