Skip to content
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

Fix ConcurrencyTest that needs to clone the client for thread-safety #189

Merged
merged 1 commit into from
Apr 6, 2021
Merged

Fix ConcurrencyTest that needs to clone the client for thread-safety #189

merged 1 commit into from
Apr 6, 2021

Conversation

dylanahsmith
Copy link
Collaborator

@dylanahsmith dylanahsmith commented Apr 1, 2021 •
edited
Loading

Based on #186 to fix CI, see dylanahsmith/memcached@github-actions...fix-concurrency-test for this PRs changes

Problem

I've noticed the following flaky test failure

  1) Failure:
ConcurrencyTest#test_threads_with_multi_get [/home/runner/work/memcached/memcached/test/unit/concurrency_test.rb:47]:
--- expected
+++ actual
@@ -1 +1 @@
-["v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11"]
+["v11", "v1", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11", "v11"]

which I was able to reproduce quite reliably locally by wrapping the test in an 100.times do block.

Looking at the concurrency tests, I noticed that they are using the same client instance across threads, but the README's Threading section says

memcached is threadsafe, but each thread requires its own Memcached instance. Create a global Memcached, and then call Memcached#clone each time you spawn a thread.

so this is what should be being tested for thread-safety.

Solution

I changed all the ConcurrencyTest methods to use the clone method as recommended in the README and it fixed the flaky test.

@dylanahsmith dylanahsmith force-pushed the fix-concurrency-test branch from 0ec2b49 to 67f84ac Compare April 6, 2021 18:46
@dylanahsmith
Copy link
Collaborator Author

@casperisfine for review

@dylanahsmith dylanahsmith force-pushed the fix-concurrency-test branch from 67f84ac to 04c8f99 Compare April 6, 2021 20:47
@dylanahsmith dylanahsmith merged commit 17bd81a into arthurnn:master Apr 6, 2021
@dylanahsmith dylanahsmith deleted the fix-concurrency-test branch April 6, 2021 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@casperisfine casperisfine casperisfine approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dylanahsmith @casperisfine