8000 [Lock] Fix race condition in tests between cache and lock component by jderusse · Pull Request #23958 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Lock] Fix race condition in tests between cache and lock component #23958

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[Lock] Fix race condition in tests between cache and lock component #23958

wants to merge 1 commit into from

Conversation

jderusse
Copy link
Member
@jderusse jderusse commented Aug 23, 2017 •
edited
Loading
Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

Currently trying to fix

  • php 5.5 in testSaveWithDifferentResources "Failed asserting that false is true"
  • php 5.5 in testSaveWithDifferentKeysOnSameResources "The store shouldn't save the second key"

Workflow:

@jderusse jderusse force-pushed the lock-fix-php56-memcached branch 10 times, most recently from bf0ed7b to 0e4ca67 Compare August 23, 2017 14:04
@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Aug 23, 2017
@jderusse jderusse force-pushed the lock-fix-php56-memcached branch 17 times, most recently from eb6c9ee to 03fce7e Compare August 23, 2017 21:33
@jderusse jderusse force-pushed the lock-fix-php56-memcached branch 4 times, most recently from 598be05 to bdae8d7 Compare August 23, 2017 22:15
@jderusse
Copy link
Member Author

Issue spotted: This is a race condition with Cache's tests. (run in parallel in the travis test suite).

Redis is flushed (regardless the namespace) at the end of each tests. For instance

symfony/src/Symfony/Component/Cache/Tests/Adapter/AbstractRedisAdapterTest.php

Lines 42 to 46 in 18ed089

public static function tearDownAfterClass()
{
self::$redis->flushDB();
self::$redis = null;
}

Memcache does not take the namespace into account in the doClear method

symfony/src/Symfony/Component/Cache/Traits/MemcachedTrait.php

Lines 234 to 237 in 18ed089

protected function doClear($namespace)
{
return $this->checkResultCode($this->client->flush());
}
. This method is called several time by the method testBasicUsage, testClear, testClearWithDeferredItems and all other tests due to the call in the teardown method.

For the redis tests, I think we can remove the usage of flushDb in tests as performed in my PR. But for memcached, I have no replacement ping @nicolas-grekas. (BTW is it expected that calling clear on a namespaces adapter removes items from other namespaces?)

Other fix: do not run Cache and Lock in parallele.

8000
@jderusse
Copy link
Member Author

Memecached fixed by #23969

@jderusse jderusse force-pushed the lock-fix-php56-memcached branch 5 times, most recently from f594d3e to ecc7ca5 Compare September 3, 2017 12:38
@jderusse jderusse force-pushed the lock-fix-php56-memcached branch from ecc7ca5 to e6f3fff Compare September 3, 2017 12:45
@jderusse jderusse changed the title [WIP] [Lock] Fix php 5 + memcached test faillure [Lock] Fix race condition in tests between cache and lock component Sep 3, 2017
@nicolas-grekas nicolas-grekas modified the milestones: 3.4, 3.3 Sep 3, 2017
@nicolas-grekas
Copy link
Member

Thank you @jderusse.

nicolas-grekas added a commit that referenced this pull request Sep 3, 2017
@nicolas-grekas
minor #23958 [Lock] Fix race condition in tests between cache and loc…
c4ecb9c 
…k component (jderusse)

This PR was submitted for the 3.4 branch but it was merged into the 3.3 branch instead (closes #23958).

Discussion
----------

[Lock] Fix race condition in tests between cache and lock component

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

Currently trying to fix
* [x] php 5.5 in testSaveWithDifferentResources "Failed asserting that false is true"
* [x] php 5.5 in testSaveWithDifferentKeysOnSameResources "The store shouldn't save the second key"

Workflow:
* [x] find a reproducer
* [x] fix memcached tests => #23969
* [x] fix redis tests => this PR

Commits
-------

26948cf Fix race condition in tests between cache and lock
@nicolas-grekas
Copy link
Member

Merged in 3.3

@jderusse jderusse deleted the lock-fix-php56-memcached branch August 2, 2019 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug Status: Needs Review
Projects
None yet
Milestone
3.3
Development

Successfully merging this pull request may close these issues.
3 participants
34FD @jderusse @nicolas-grekas @carsonbot
0