ManagesFailPointsTrait and always stop event listeners #1
Conversation
|
|
||
| trait ManagesFailPointsTrait | ||
| { | ||
| /** @var list<list{string, Server}> */ |
There was a problem hiding this comment.
In your original PR you were storing the entire configureFailPoint command document. We only need the name of the fail point, so I've reduced this to a tuple like we originally had in FailPointObserver.
| { | ||
| assertObjectHasAttribute('configureFailPoint', $failPoint); | ||
| assertIsString($failPoint->configureFailPoint); | ||
| assertObjectHasAttribute('mode', $failPoint); |
There was a problem hiding this comment.
A few assertions before we access object properties. I had these in for testing, but I suppose you can remove them if want to rely on the command succeeding.
| $operation = new DatabaseCommand('admin', $failPoint); | ||
| $operation->execute($server); | ||
|
|
||
| if ($failPoint->mode !== 'off') { |
There was a problem hiding this comment.
I don't believe this is used in any tests, but it seemed reasonable to check for it since there's no point in disabling a fail point twice.
| $context->stopEventObservers(); | ||
| $context->stopEventCollectors(); | ||
| } finally { | ||
| $this->disableFailPoints($context->getFailPoints()); | ||
| $context->disableFailPoints(); |
There was a problem hiding this comment.
Note that I moved stopEventObservers() and stopEventCollectors() within the finally block. That seemed correct given the following text from Executing a Test in the Unified Test Format spec:
For each element in test.operations, follow the process in Executing an Operation. If an unexpected error is encountered or an assertion fails, the test runner MUST consider this test to have failed.
If any event listeners were enabled on any client entities, the test runner MUST now disable those event listeners.
If any fail points were configured, the test runner MUST now disable those fail points (on the same server)...
It's possible that this could have resulted in memory leaks after failing tests. The subscriber would never get unregistered, and since they were registered globally they'd continue to accumulate in the process. Both EventCollector and EventObserver also hold a reference to a Context object, so those would also linger. I'm not sure if gc_collect_cycles() would have helped here, since it's PHPC itself that ultimately holds a reference to the subscriber(s) in its request-scoped HashTable.
I put this change in a separate commit, so if you think it deserves its own PHPLIB ticket for tracking feel free to create that and incorporate it into the message when merging this.
@alcaeus: This is for mongodb#1427