Skip to content

Commit ce0a91a

Browse files
JacobBrownAustinJacobBrownAustin
committed
ACPT-1666: Fix race conditions in _resetState methods
* WeakMapSorter arguments from root di.xml have been temporarily moved into Config Module - This should be moved back to the root di.xml once the issue with array_replace_recursive is fixed in lib/internal/Magento/Framework/ObjectManager/Config/Config.php * Changed Magento\Framework\TestFramework\ApplicationStateComparator\ObjectManagerInterface and dependant classes so that we just use _resetState() instead of the individual resetStateWeakMapObjects() and resetStateSharedInstances() * In ReloadProcessorComposite, moved ksort to constructor so that it only has to be called once. * In Resetter, made new constant RESET_PATH * Now using ObjectManager from setObjectManager to construct WeakMapSorter to avoid use of global variable. * Moved WeakMapSorter to property in Resetter instead of local variable so that we can cache the sortOrder values which are added dynamically. * Changed WeakMapSorter's use of di.xml so that the keys are the class names and the sortValues are the values. * WeakMapSorter now uses and caches the sortValues for subclasses and implementations of interfaces. * Magento\Framework\TestFramework\ApplicationStateComparator\DynamicFactoryDecorator now uses Resetter
1 parent 3ae55bc commit ce0a91a

File tree

13 files changed

+223
-158
lines changed

13 files changed

+223
-158
lines changed

‎app/code/Magento/Config/etc/di.xml

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -397,4 +397,16 @@
397397
</argument>
398398
</arguments>
399399
</type>
400+
<!-- TODO: Move these to Framework's di.xml after issue with array_replace_recursive in configuration are resolved.
401+
-->
402+
<type name="Magento\Framework\ObjectManager\Resetter\WeakMapSorter">
403+
<arguments>
404+
<argument name="sortOrder" xsi:type="array">
405+
<item name="Magento\Framework\Session\SessionManagerInterface" xsi:type="number">1000</item>
406+
<item name="Magento\Framework\App\Cache\State" xsi:type="number">10000</item>
407+
<item name="Magento\Framework\DB\Logger\LoggerProxy" xsi:type="number">9999</item>
408+
<item name="Magento\Framework\DB\Adapter\Pdo\Mysql" xsi:type="number">9999</item>
409+
</argument>
410+
</arguments>
411+
</type>
400412
</config>

‎dev/tests/integration/framework/Magento/TestFramework/ApplicationStateComparator/_files/state-skip-list.php

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,9 @@
88
/* These classes are skipped completely during comparison. */
99
return [
1010
'*' => [
11+
Magento\Store\Model\ResourceModel\Group\Collection\FetchStrategy::class => null,
12+
13+
Magento\Framework\ObjectManager\Resetter\WeakMapSorter::class => null,
1114
// phpcs:disable Generic.Files.LineLength.TooLong
1215
// list of the latest failures started
1316
Magento\Sales\Api\Data\ShippingAssignmentInterfaceFactory::class => null,

‎dev/tests/integration/testsuite/Magento/GraphQl/App/State/GraphQlStateDiff.php

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ public function __construct()
6666
AppObjectManager::setInstance($this->objectManagerForTest);
6767
Bootstrap::setObjectManager($this->objectManagerForTest);
6868
$this->comparator = $this->objectManagerForTest->create(Comparator::class);
69-
$this->objectManagerForTest->resetStateSharedInstances();
69+
$this->objectManagerForTest->_resetState();
7070
}
7171

7272
/**
@@ -172,10 +172,10 @@ private function request(
172172
TestCase $test,
173173
bool $firstRequest = false
174174
): string {
175-
$this->objectManagerForTest->resetStateSharedInstances();
175+
$this->objectManagerForTest->_resetState();
176176
$this->comparator->rememberObjectsStateBefore($firstRequest);
177177
$response = $this->doRequest($query, $authInfo);
178-
$this->objectManagerForTest->resetStateSharedInstances();
178+
$this->objectManagerForTest->_resetState();
179179
$this->comparator->rememberObjectsStateAfter($firstRequest);
180180
$result = $this->comparator->compareBetweenRequests($operationName);
181181
$test->assertEmpty(

‎lib/internal/Magento/Framework/App/State/ReloadProcessorComposite.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,14 +16,14 @@ class ReloadProcessorComposite implements ReloadProcessorInterface
1616
*/
1717
public function __construct(private array $processors)
1818
{
19+
ksort($this->processors, SORT_STRING);
1920
}
2021

2122
/**
2223
* @inheritdoc
2324
*/
2425
public function reloadState(): void
2526
{
26-
ksort($this->processors);
2727
/** @var ReloadProcessorInterface $processor */
2828
foreach ($this->processors as $processor) {
2929
$processor->reloadState();

‎lib/internal/Magento/Framework/ObjectManager/Resetter/Resetter.php

Lines changed: 20 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,17 @@
1818
*/
1919
class Resetter implements ResetterInterface
2020
{
21+
public const RESET_PATH = '/app/etc/reset.php';
22+
2123
/** @var WeakMap instances to be reset after request */
2224
private WeakMap $resetAfterWeakMap;
2325

26+
/** @var ObjectManagerInterface Note: We use temporal coupling here because of chicken/egg during bootstrapping */
27+
private ObjectManagerInterface $objectManager;
28+
29+
/** @var WeakMapSorter|null Note: We use temporal coupling here because of chicken/egg during bootstrapping */
30+
private ?WeakMapSorter $weakMapSorter = null;
31+
2432
/**
2533
* @var array
2634
*
@@ -50,9 +58,9 @@ class Resetter implements ResetterInterface
5058
*/
5159
public function __construct()
5260
{
53-
if (\file_exists(BP . '/app/etc/reset.php')) {
61+
if (\file_exists(BP . self::RESET_PATH)) {
5462
// phpcs:ignore Magento2.Security.IncludeFile.FoundIncludeFile
55-
$this->classList = array_replace($this->classList, (require BP . '/app/etc/reset.php'));
63+
$this->classList = array_replace($this->classList, (require BP . self::RESET_PATH));
5664
}
5765
$this->resetAfterWeakMap = new WeakMap;
5866
}
@@ -65,9 +73,7 @@ public function __construct()
6573
*/
6674
public function addInstance(object $instance) : void
6775
{
68-
if ($instance instanceof ResetAfterRequestInterface
69-
|| isset($this->classList[\get_class($instance)])
70-
) {
76+
if ($instance instanceof ResetAfterRequestInterface || isset($this->classList[\get_class($instance)])) {
7177
$this->resetAfterWeakMap[$instance] = true;
7278
}
7379
}
@@ -83,43 +89,31 @@ public function _resetState(): void
8389
/* Note: We force garbage collection to clean up cyclic referenced objects before _resetState()
8490
* This is to prevent calling _resetState() on objects that will be destroyed by garbage collector. */
8591
gc_collect_cycles();
86-
$weakMapSorter = ObjectManager::getInstance()
87-
->create(WeakMapSorter::class, ['weakmap' => $this->resetAfterWeakMap]);
88-
$weakMapSorter->_resetState();
89-
$temporaryWeakReferenceList = [];
90-
foreach ($this->resetAfterWeakMap as $weakMapObject => $value) {
91-
$temporaryWeakReferenceList[] = WeakReference::create($weakMapObject);
92-
unset($weakMapObject);
93-
unset($value);
92+
if (!$this->weakMapSorter) {
93+
$this->weakMapSorter = $this->objectManager->get(WeakMapSorter::class);
9494
}
95-
foreach ($temporaryWeakReferenceList as $weakReference) {
95+
foreach ($this->weakMapSorter->sortWeakMapIntoWeakReferenceList($this->resetAfterWeakMap) as $weakReference) {
9696
$instance = $weakReference->get();
9797
if (!$instance) {
9898
continue;
9999
}
100100
if (!$instance instanceof ResetAfterRequestInterface) {
101101
$this->resetStateWithReflection($instance);
102+
} else {
103+
$instance->_resetState();
102104
}
103105
}
104-
unset($temporaryWeakReferenceList);
106+
/* Note: We must force garbage collection to clean up cyclic referenced objects after _resetState()
107+
* Otherwise, they may still show up in the WeakMap. */
108+
gc_collect_cycles();
105109
}
106110

107111
/**
108112
* @inheritDoc
109-
* @phpcs:disable Magento2.CodeAnalysis.EmptyBlock.DetectedFunction
110113
*/
111114
public function setObjectManager(ObjectManagerInterface $objectManager) : void
112115
{
113-
}
114-
115-
/**
116-
* Gets weak map
117-
*
118-
* @return WeakMap
119-
*/
120-
public function getWeakMap() : WeakMap
121-
{
122-
throw new \RuntimeException("Not implemented\n");
116+
$this->objectManager = $objectManager;
123117
}
124118

125119
/**
@@ -132,10 +126,8 @@ public function getWeakMap() : WeakMap
132126
private function resetStateWithReflection(object $instance)
133127
{
134128
$className = \get_class($instance);
135-
136129
$reflectionClass = $this->reflectionCache[$className]
137130
?? $this->reflectionCache[$className] = new \ReflectionClass($className);
138-
139131
foreach ($reflectionClass->getProperties() as $property) {
140132
$type = $property->getType()?->getName();
141133
if (empty($type) && preg_match('/@var\s+([^\s]+)/', $property->getDocComment(), $matches)) {
@@ -144,7 +136,6 @@ private function resetStateWithReflection(object $instance)
144136
$type = 'array';
145137
}
146138
}
147-
148139
$name = $property->getName();
149140
if (!in_array($type, ['bool', 'array', 'null', 'true', 'false'], true)) {
150141
continue;

‎lib/internal/Magento/Framework/ObjectManager/Resetter/ResetterInterface.php

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -31,11 +31,4 @@ public function addInstance(object $instance) : void;
3131
* @return void
3232
*/
3333
public function setObjectManager(ObjectManagerInterface $objectManager) : void;
34-
35-
/**
36-
* Gets weak map
37-
*
38-
* @return WeakMap
39-
*/
40-
public function getWeakMap() : WeakMap;
4134
}

‎lib/internal/Magento/Framework/ObjectManager/Resetter/SortableReferenceObject.php

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
/**
1414
* Data class used for hold reference and sort value
1515
*/
16-
class SortableReferenceObject implements ResetAfterRequestInterface
16+
class SortableReferenceObject
1717
{
1818
/**
1919
* @param WeakReference $reference
@@ -35,17 +35,8 @@ public function getSort() : int
3535
return $this->sort;
3636
}
3737

38-
/**
39-
* State reset
40-
*
41-
* @return void
42-
*/
43-
public function _resetState(): void
38+
public function getWeakReference() : WeakReference
4439
{
45-
$object = $this->reference->get();
46-
if (!$object || !($object instanceof ResetAfterRequestInterface)) {
47-
return;
48-
}
49-
$object->_resetState();
40+
return $this->reference;
5041
}
5142
}

‎lib/internal/Magento/Framework/ObjectManager/Resetter/WeakMapSorter.php

Lines changed: 47 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,12 @@
1515
/**
1616
* Sorts a WeakMap into an ordered array of WeakReference and reset them in order.
1717
*/
18-
class WeakMapSorter implements ResetAfterRequestInterface
18+
class WeakMapSorter
1919
{
2020

21-
private const DEFAULT_SORT_VALUE = 5000;
21+
public const DEFAULT_SORT_VALUE = 5000;
2222

23-
private const MAX_SORT_VALUE = 9000;
23+
public const MAX_SORT_VALUE = 10000;
2424

2525
/**
2626
* @var SortableReferenceObject[]
@@ -30,46 +30,69 @@ class WeakMapSorter implements ResetAfterRequestInterface
3030
/**
3131
* Constructor
3232
*
33-
* @param WeakMap $weakmap
33+
* @param array<string, int> $sortOrder
3434
* @SuppressWarnings(PHPMD.UnusedLocalVariable)
3535
*/
36-
public function __construct (WeakMap $weakmap, array $resettableArgs)
36+
public function __construct (private array $sortOrder)
3737
{
38+
// Note: Even though they are declared as xsi:type="number", they are still strings, so we convert them here.
39+
foreach ($this->sortOrder as &$value) {
40+
$value = (int)$value;
41+
}
42+
}
43+
44+
/**
45+
* @param WeakMap $weakmap
46+
* @return WeakReference[]
47+
*/
48+
public function sortWeakMapIntoWeakReferenceList(WeakMap $weakmap) : array
49+
{
50+
/** @var SortableReferenceObject[] */
51+
$sortableReferenceList = [];
3852
foreach ($weakmap as $weakMapObject => $value) {
39-
if (!$weakMapObject || !($weakMapObject instanceof ResetAfterRequestInterface)) {
53+
if (!$weakMapObject) {
4054
continue;
4155
}
42-
$sortValue = $this->getSortValueOfObject($weakMapObject, $resettableArgs);
56+
$sortValue = $this->getSortValueOfObject($weakMapObject);
4357
$weakReference = WeakReference::create($weakMapObject);
44-
$this->sortableReferenceList[] = new SortableReferenceObject($weakReference, $sortValue);
58+
$sortableReferenceList[] = new SortableReferenceObject($weakReference, $sortValue);
4559
}
4660
usort(
47-
$this->sortableReferenceList,
61+
$sortableReferenceList,
4862
fn(SortableReferenceObject $a, SortableReferenceObject $b) => $a->getSort() - $b->getSort()
4963
);
64+
$returnValue = [];
65+
foreach ($sortableReferenceList as $sortableReference) {
66+
$returnValue[] = $sortableReference->getWeakReference();
67+
}
68+
return $returnValue;
5069
}
5170

5271
/**
5372
* @param object $object
5473
* @return int
5574
*/
56-
public function getSortValueOfObject(object $object, array $resettableArgs) : int
75+
private function getSortValueOfObject(object $object) : int
5776
{
58-
if (!in_array($object, $resettableArgs)) {
59-
return static::DEFAULT_SORT_VALUE;
77+
$className = get_class($object);
78+
if (array_key_exists($className , $this->sortOrder)) {
79+
return $this->sortOrder[$className];
6080
}
61-
$args = ObjectManager::getInstance()->get(\Magento\Framework\ObjectManager\Config\Config::class)->getArguments(get_class($object));
62-
63-
return self::MAX_SORT_VALUE;
64-
}
65-
66-
/**
67-
* @inheritDoc
68-
*/
69-
public function _resetState(): void
70-
{
71-
foreach ($this->sortableReferenceList as $sortableReferenceObject) {
72-
$sortableReferenceObject->_resetState();
81+
for ($parentClass = $className; $parentClass = get_parent_class($parentClass);) {
82+
if (array_key_exists($parentClass , $this->sortOrder)) {
83+
$sortValue = $this->sortOrder[$parentClass];
84+
$this->sortOrder[$className] = $sortValue;
85+
return $sortValue;
86+
}
87+
}
88+
$sortValue = static::DEFAULT_SORT_VALUE;
89+
foreach ($this->sortOrder as $sortOrderKey => $sortOrderValue) {
90+
if ($object instanceof $sortOrderKey) {
91+
$sortValue = $sortOrderValue;
92+
break;
93+
}
7394
}
95+
$this->sortOrder[$className] = $sortValue;
96+
return $sortValue;
7497
}
7598
}

‎lib/internal/Magento/Framework/TestFramework/ApplicationStateComparator/Collector.php

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ function ($element) use (
9292
*/
9393
public function getSharedObjects(string $shouldResetState): array
9494
{
95-
if ($this->objectManager instanceof ObjectManagerInterface) {
95+
if ($this->objectManager instanceof StateObjectManagerInterface) {
9696
$sharedInstances = $this->objectManager->getSharedInstances();
9797
} else {
9898
$obj = new \ReflectionObject($this->objectManager);
@@ -139,9 +139,9 @@ public function getPropertiesConstructedAndCurrent(): array
139139
throw new \Exception("Not the correct type of ObjectManager");
140140
}
141141
// Calling _resetState helps us avoid adding skip/filter for these classes.
142-
$objectManager->resetStateWeakMapObjects();
142+
$objectManager->_resetState();
143143
$objects = [];
144-
foreach ($objectManager->getWeakMap() as $object => $propertiesBefore) {
144+
foreach ($objectManager->getResetter()->getCollectedWeakMap() as $object => $propertiesBefore) {
145145
$objects[] = new CollectedObjectConstructedAndCurrent(
146146
$object,
147147
$propertiesBefore,

0 commit comments

Comments
 (0)