Bug #33621

Filebackend reports error when trying to rename file due to concurrent proccesses

Added by Peter Russ almost 8 years ago. Updated over 5 years ago.

Status:
Resolved
Priority:
Should have
Category:
Cache
Start date:
2012-02-02
Due date:
% Done:

100%

PHP Version:
5.3
Has patch:
Yes
Complexity:

Description

Due to concurrent processes it happens that a temp-cache-file gets renamed while an other process is just writing it.
Further there is an error in the error handling.

We fixed both.
  1. added an exclusive lock for the writing operation
  2. added counters for a break to avoid an endless loop due to file system errors
  3. added timers for getting the process to sleep
  4. fixed error handling

33621.patch View (3.07 KB) Peter Russ, 2012-02-02 14:21

33621_SimpleFileBackend.patch View (844 Bytes) Mariusz Maroszczyk, 2013-09-30 14:46


Related issues

Related to TYPO3.Flow - Bug #27989: Wrong check in our atomic writes code Resolved 2011-07-08

Associated revisions

Revision 54486a01 (diff)
Added by Christopher Hlubek over 6 years ago

[BUGFIX] Fix race condition when setting cache entries

This change adds the uniqid() to the temporary filename again and adds
the process id (if the function "posix_getpid" is present) as another
unique identifier.

Change-Id: Ib2a21f8607f464901b81c1950d112f639cbe29ba
Fixes: #33621
Releases: master, 2.0

History

#1 Updated by Peter Russ almost 8 years ago

#2 Updated by Peter Russ almost 8 years ago

  • % Done changed from 0 to 80

Reported also fixes to TYPO3 s. http://forge.typo3.org/issues/33622

#3 Updated by Karsten Dambekalns almost 8 years ago

Hm, I had this in the pipeline: https://review.typo3.org/8786

#4 Updated by Karsten Dambekalns almost 8 years ago

  • Category set to Cache
  • Status changed from New to Accepted
  • Assignee set to Karsten Dambekalns

#5 Updated by Karsten Dambekalns almost 8 years ago

Thanks! Aside from a simple issue with your code (forgot to throw a new exception), this seems fine. Two "theoretical" problems, though:

  1. the unit tests fail, as we use fvsStream to mock the filesystem, but https://bugs.php.net/60232 breaks lock support for custom stream wrappers
  2. Robert did not use LOCK_EX back in the days, because it is said to be useless as soon as you don't use a local filesystem (but NFS e.g.)

Any ideas about the former (except using the real FS for the tests) and any hard facts about the latter?

#6 Updated by Karsten Dambekalns almost 8 years ago

  • Status changed from Accepted to Needs Feedback

#7 Updated by Gerrit Code Review almost 8 years ago

  • Status changed from Needs Feedback to Under Review

Patch set 3 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/8931

#8 Updated by Karsten Dambekalns over 7 years ago

  • Target version changed from 1.0.3 to 1.0.4

#9 Updated by Karsten Dambekalns over 7 years ago

  • Status changed from Under Review to Needs Feedback

#10 Updated by Karsten Dambekalns over 7 years ago

  • Target version changed from 1.0.4 to 1.0.5

#11 Updated by Karsten Dambekalns over 7 years ago

  • Target version changed from 1.0.5 to 1.1 RC1

#12 Updated by Karsten Dambekalns over 7 years ago

  • Target version changed from 1.1 RC1 to 2.0 beta 1

#13 Updated by Karsten Dambekalns almost 7 years ago

  • Target version changed from 2.0 beta 1 to 2.0

#14 Updated by Christopher Hlubek over 6 years ago

  • Status changed from Needs Feedback to Accepted
  • Assignee changed from Karsten Dambekalns to Christopher Hlubek

We still have this issue with the latest session implementation. With the iterable file backends this got even worse because no uniqid() is used anymore.

I would propose a simpler change that includes the process id in the uniqid and excludes temporary files from the backend iterator. This should solve concurrent process race conditions and needs no locking.

#15 Updated by Gerrit Code Review over 6 years ago

  • Status changed from Accepted to Under Review

Patch set 1 for branch 2.0 has been pushed to the review server.
It is available at https://review.typo3.org/23081

#16 Updated by Karsten Dambekalns over 6 years ago

  • Target version changed from 2.0 to 2.0.1

#17 Updated by Gerrit Code Review over 6 years ago

Patch set 1 for branch master has been pushed to the review server.
It is available at https://review.typo3.org/23102

#18 Updated by Gerrit Code Review over 6 years ago

Patch set 2 for branch master has been pushed to the review server.
It is available at https://review.typo3.org/23102

#19 Updated by Gerrit Code Review over 6 years ago

Patch set 2 for branch 2.0 has been pushed to the review server.
It is available at https://review.typo3.org/23081

#20 Updated by Mariusz Maroszczyk about 6 years ago

The SimpleFileBackend "set" method also does not handle concurrency. "rename" should only be called if the file still exists (wasn't renamed yet).
A simple condition fixes the problem:

if (file_exists($temporaryCacheEntryPathAndFilename)) {
   rename($temporaryCacheEntryPathAndFilename, $cacheEntryPathAndFilename);
}

#21 Updated by Bastian Waidelich about 6 years ago

There is still an issue with the current implementation (even with the patches applied): rename() issues a warning so

$i = 0;
$cacheEntryPathAndFilename = $this->cacheDirectory . $entryIdentifier . $this->cacheEntryFileExtension;
while (($result = rename($temporaryCacheEntryPathAndFilename, $cacheEntryPathAndFilename)) === FALSE && $i < 5) {
    $i++;
}
if ($result === FALSE) {
    throw new \TYPO3\Flow\Cache\Exception('The cache file "' . $cacheEntryPathAndFilename . '" could not be written.', 1222361632);
}

won't work as expected because a warning is issued for the first failed attempt leading to an exception in dev context. Besides this loop is probably iterated much too fast. We should add a usleep().

#22 Updated by Christopher Hlubek about 6 years ago

  • Assignee changed from Christopher Hlubek to Bastian Waidelich

Bastian: Can you push a new patchset that fixes the rename warning (and maybe add the usleep to the busy waiting)?

Bastian Waidelich wrote:

There is still an issue with the current implementation (even with the patches applied): rename() issues a warning so
[...]

won't work as expected because a warning is issued for the first failed attempt leading to an exception in dev context. Besides this loop is probably iterated much too fast. We should add a usleep().

#23 Updated by Christopher Hlubek about 6 years ago

I would say that could be fixed by ignoring warnings from the rename (which we have to do anyway as Basti stated). An additional file_exists call is bad in terms of performance. And it doesn't give any guarantees since the calls to file_exists and rename are not atomic and race conditions could occur where the file existed but is gone when calling rename.

Mariusz Maroszczyk wrote:

The SimpleFileBackend "set" method also does not handle concurrency. "rename" should only be called if the file still exists (wasn't renamed yet).
A simple condition fixes the problem:
[...]

#24 Updated by Gerrit Code Review over 5 years ago

Patch set 3 for branch master of project Packages/TYPO3.Flow has been pushed to the review server.
It is available at https://review.typo3.org/23102

#25 Updated by Karsten Dambekalns over 5 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 80 to 100

Also available in: Atom PDF