Issue 252 in memcached: testapp.c sometimes fails due to race condition

[memc...@googlecode.com]

Status: New
Owner: ----
Labels: Type-Defect Priority-Medium

New issue 252 by superama...@gmail.com: testapp.c sometimes fails due to
race condition
http://code.google.com/p/memcached/issues/detail?id=252

What steps will reproduce the problem?
1. build memcached, run 'make check' and then ./testapp
2. Repeat many many times. Seems to fail more often on linux ppc64 and os x

Alternatively apply this patch to create an artificial race:

diff --git a/memcached.c b/memcached.c
index b121e16..678690b 100644
--- a/memcached.c
+++ b/memcached.c
@@ -4604,6 +4604,8 @@ static void save_pid(const char *pid_file) {
return;
}

+ usleep(25);
+
fprintf(fp,"%ld\n", (long)getpid());
if (fclose(fp) == -1) {
vperror("Could not close the pid file %s", pid_file);

Error will be something like:

testapp: testapp.c:397: start_server: Assertion `fgets(buffer,
sizeof(buffer), fp) != ((void *)0)' failed.

What version of the product are you using? On what operating system?

Have seen this on 1.4.13, and trunk, on Ubuntu.

Note that there is even a comment in this test that acknowledges the
problem:

/* loop and wait for the pid file.. There is a potential race
* condition that the server just created the file but isn't
* finished writing the content, but I'll take the chance....
*/

The attached patch solves the issue by retrying 5 times with 10us sleeps in
between.

Attachments:
fix-racey-test.patch 1.2 KB

[memc...@googlecode.com]

Comment #1 on issue 252 by ringo.p...@gmail.com: testapp.c sometimes fails

due to race condition
http://code.google.com/p/memcached/issues/detail?id=252

Discussed with Clint on irc - this is the patch we are using for the
official Ubuntu 12.04 memcached package.

Attachments:
60_fix_racey_test.patch 1.2 KB

[memc...@googlecode.com]

Updates:
Status: Fixed

Comment #2 on issue 252 by dorma...@rydia.net: testapp.c sometimes fails

due to race condition
http://code.google.com/p/memcached/issues/detail?id=252

Should've had a failsafe at the end to still assert on NULL but that's
pedantic.

should hopefully have the daemonization process made more robust in the
next few cuts and it won't matter. thanks!