[BUGFIX] getTreeList inserts duplicate keys in cache_treelist 27/59127/6
authorAlexander Schnitzler <git@alexanderschnitzler.de>
Wed, 12 Dec 2018 21:53:07 +0000 (22:53 +0100)
committerBenni Mack <benni@typo3.org>
Thu, 13 Dec 2018 17:55:06 +0000 (18:55 +0100)
Unfortunately https://review.typo3.org/58951/ did not actually
solve issues #86028 and #86491 for good.

There are two issues concerning the former approach:

1) The expiration time of all created caches was 0, which resulted
   in a permanent creation and deletion of cache entries. This
   behaviour cannot be called caching.

2) Number 1) increases the chance for race conditions where several
   parallel requests tried to create the same cache entry.

To fix this, the check for an existing cache entry will be reverted
to behave like before the regression, i.e. cache entries with an
expiration timestamp of 0 are considered valid again.

Also, new caches are created within a transaction, which prevents
duplicate key errors.

Releases: master, 8.7
Resolves: #87139
Change-Id: If9470f6e0f875c0ec4fe3c092c9bd0dfc059de2d
Reviewed-on: https://review.typo3.org/59127
Reviewed-by: Markus Klein <markus.klein@typo3.org>
Tested-by: TYPO3com <no-reply@typo3.com>
Reviewed-by: Benni Mack <benni@typo3.org>
Reviewed-by: Andreas Fernandez <a.fernandez@scripting-base.de>
Tested-by: Andreas Fernandez <a.fernandez@scripting-base.de>
Tested-by: Benni Mack <benni@typo3.org>
typo3/sysext/frontend/Classes/ContentObject/ContentObjectRenderer.php

index d5ee365..766b45c 100644 (file)
@@ -6249,36 +6249,30 @@ class ContentObjectRenderer implements LoggerAwareInterface
                 GeneralUtility::makeInstance(Context::class)->getPropertyFromAspect('frontend.user', 'groupIds', [0, -1])
             ];
             $requestHash = md5(serialize($parameters));
-
-            $cacheTreeListConnection = GeneralUtility::makeInstance(ConnectionPool::class)
-                ->getConnectionForTable('cache_treelist');
-
-            $queryBuilder = $cacheTreeListConnection->createQueryBuilder();
-            $query = $queryBuilder->select('treelist', 'expires')
+            $queryBuilder = GeneralUtility::makeInstance(ConnectionPool::class)
+                ->getQueryBuilderForTable('cache_treelist');
+            $cacheEntry = $queryBuilder->select('treelist')
                 ->from('cache_treelist')
                 ->where(
                     $queryBuilder->expr()->eq(
                         'md5hash',
                         $queryBuilder->createNamedParameter($requestHash, \PDO::PARAM_STR)
+                    ),
+                    $queryBuilder->expr()->orX(
+                        $queryBuilder->expr()->gt(
+                            'expires',
+                            $queryBuilder->createNamedParameter($GLOBALS['EXEC_TIME'], \PDO::PARAM_INT)
+                        ),
+                        $queryBuilder->expr()->eq('expires', $queryBuilder->createNamedParameter(0, \PDO::PARAM_INT))
                     )
                 )
-                ->setMaxResults(1);
-
-            $cacheEntry = $query->execute()->fetch();
-            $cacheEntryExists = is_array($cacheEntry);
-            $cacheEntryIsExpired = $cacheEntry['expires'] <= $GLOBALS['EXEC_TIME'];
+                ->setMaxResults(1)
+                ->execute()
+                ->fetch();
 
-            if ($cacheEntryExists) {
-                if (!$cacheEntryIsExpired) {
-                    // Cache hit
-                    return $cacheEntry['treelist'];
-                }
-                $cacheTreeListConnection->delete(
-                    'cache_treelist',
-                    [
-                        'md5hash' => $requestHash
-                    ]
-                );
+            if (is_array($cacheEntry)) {
+                // Cache hit
+                return $cacheEntry['treelist'];
             }
             // If Id less than zero it means we should add the real id to list:
             if ($id < 0) {
@@ -6426,15 +6420,21 @@ class ContentObjectRenderer implements LoggerAwareInterface
                     $theList[] = $addId;
                 }
             }
-            GeneralUtility::makeInstance(ConnectionPool::class)->getConnectionForTable('cache_treelist')->insert(
-                'cache_treelist',
-                [
-                    'md5hash' => $requestHash,
-                    'pid' => $id,
-                    'treelist' => implode(',', $theList),
-                    'tstamp' => $GLOBALS['EXEC_TIME']
-                ]
-            );
+
+            $cacheEntry = [
+                'md5hash' => $requestHash,
+                'pid' => $id,
+                'treelist' => implode(',', $theList),
+                'tstamp' => $GLOBALS['EXEC_TIME'],
+            ];
+
+            $connection = GeneralUtility::makeInstance(ConnectionPool::class)->getConnectionForTable('cache_treelist');
+            try {
+                $connection->transactional(function ($connection) use ($cacheEntry) {
+                    $connection->insert('cache_treelist', $cacheEntry);
+                });
+            } catch (\Throwable $e) {
+            }
         }
 
         return implode(',', $theList);