arthurnn / memcached

A Ruby interface to the libmemcached C client
Academic Free License v3.0
432 stars 127 forks source link

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

Closed dylanahsmith closed 3 years ago

dylanahsmith commented 3 years ago

Based on https://github.com/arthurnn/memcached/pull/186 to fix CI, see https://github.com/dylanahsmith/memcached/compare/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 commented 3 years ago

@casperisfine for review