Skip to content

Commit

Permalink
Fix timing bug in parallel apr_dir_make_recursive on Windows.
Browse files Browse the repository at this point in the history
* file_io/win32/dir.c
  (dir_make_parent): When parent just got created, continue creating children.
  (apr_dir_make_recursive): Only handle EEXIST of the requested directory as
   success, not any ancestor.

Patch by: rhuijben

* test/testdir.c
  (test_mkdir_recurs_parallel): New multithreaded test case.
  (test_removeall): Clean up after test_mkdir_recurs_parallel.


git-svn-id: https://svn.apache.org/repos/asf/apr/apr/trunk@1559873 13f79535-47bb-0310-9956-ffa450edef68
  • Loading branch information
brainy committed Jan 21, 2014
1 parent 2f03ba2 commit 1d7f658
Show file tree
Hide file tree
Showing 2 changed files with 146 additions and 17 deletions.
41 changes: 24 additions & 17 deletions file_io/win32/dir.c
Original file line number Diff line number Diff line change
Expand Up @@ -316,8 +316,8 @@ static apr_status_t dir_make_parent(char *path,
if (APR_STATUS_IS_ENOENT(rv)) { /* Missing an intermediate dir */
rv = dir_make_parent(path, perm, pool);

if (rv == APR_SUCCESS) {
rv = apr_dir_make (path, perm, pool); /* And complete the path */
if (rv == APR_SUCCESS || APR_STATUS_IS_EEXIST(rv)) {
rv = apr_dir_make(path, perm, pool); /* And complete the path */
}
}

Expand All @@ -330,28 +330,35 @@ APR_DECLARE(apr_status_t) apr_dir_make_recursive(const char *path,
apr_pool_t *pool)
{
apr_status_t rv = 0;

rv = apr_dir_make (path, perm, pool); /* Try to make PATH right out */

if (APR_STATUS_IS_ENOENT(rv)) { /* Missing an intermediate dir */
char *dir;

rv = apr_filepath_merge(&dir, "", path, APR_FILEPATH_NATIVE, pool);

if (rv == APR_SUCCESS)
rv = dir_make_parent(dir, perm, pool); /* Make intermediate dirs */

if (rv == APR_SUCCESS)
if (rv != APR_SUCCESS)
return rv;

rv = dir_make_parent(dir, perm, pool); /* Make intermediate dirs */

if (rv == APR_SUCCESS || APR_STATUS_IS_EEXIST(rv)) {
rv = apr_dir_make (dir, perm, pool); /* And complete the path */
}

/*
* It's OK if PATH exists. Timing issues can lead to the second
* apr_dir_make being called on existing dir, therefore this check
* has to come last.
*/
if (APR_STATUS_IS_EEXIST(rv))
return APR_SUCCESS;
if (APR_STATUS_IS_EEXIST(rv)) {
rv = APR_SUCCESS; /* Timing issue; see comment below */
}
}
}
else if (APR_STATUS_IS_EEXIST(rv)) {
/*
* It's OK if PATH exists. Timing issues can lead to the
* second apr_dir_make being called on existing dir, therefore
* this check has to come last.
*/
rv = APR_SUCCESS;
}

return rv;
}
Expand Down
122 changes: 122 additions & 0 deletions test/testdir.c
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "apr_errno.h"
#include "apr_general.h"
#include "apr_lib.h"
#include "apr_thread_proc.h"
#include "testutil.h"

static void test_mkdir(abts_case *tc, void *data)
Expand Down Expand Up @@ -62,6 +63,60 @@ static void test_mkdir_recurs(abts_case *tc, void *data)
ABTS_INT_EQUAL(tc, APR_DIR, finfo.filetype);
}

static void *APR_THREAD_FUNC thread_mkdir_func(apr_thread_t *thd, void *data)
{
abts_case *tc = data;
apr_status_t s1, s2, s3, s4, s5;

s1 = apr_dir_make_recursive("data/prll/one/thwo/three",
APR_FPROT_UREAD | APR_FPROT_UWRITE | APR_FPROT_UEXECUTE,
p);
s2 = apr_dir_make_recursive("data/prll/four/five/six/seven/eight",
APR_FPROT_UREAD | APR_FPROT_UWRITE | APR_FPROT_UEXECUTE,
p);
s3 = apr_dir_make_recursive("data/prll/nine/ten",
APR_FPROT_UREAD | APR_FPROT_UWRITE | APR_FPROT_UEXECUTE,
p);
s4 = apr_dir_make_recursive("data/prll/11/12/13/14/15/16/17/18/19/20",
APR_FPROT_UREAD | APR_FPROT_UWRITE | APR_FPROT_UEXECUTE,
p);
s5 = apr_dir_make_recursive("data/fortytwo",
APR_FPROT_UREAD | APR_FPROT_UWRITE | APR_FPROT_UEXECUTE,
p);

ABTS_INT_EQUAL(tc, APR_SUCCESS, s1);
ABTS_INT_EQUAL(tc, APR_SUCCESS, s2);
ABTS_INT_EQUAL(tc, APR_SUCCESS, s3);
ABTS_INT_EQUAL(tc, APR_SUCCESS, s4);
ABTS_INT_EQUAL(tc, APR_SUCCESS, s5);
return NULL;
}

static void test_mkdir_recurs_parallel(abts_case *tc, void *data)
{
apr_thread_t *t1, *t2, *t3, *t4;
apr_status_t s1, s2, s3, s4;

s1 = apr_thread_create(&t1, NULL, thread_mkdir_func, tc, p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, s1);
s2 = apr_thread_create(&t2, NULL, thread_mkdir_func, tc, p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, s2);
s3 = apr_thread_create(&t3, NULL, thread_mkdir_func, tc, p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, s3);
s4 = apr_thread_create(&t4, NULL, thread_mkdir_func, tc, p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, s4);

apr_thread_join(&s1, t1);
apr_thread_join(&s2, t2);
apr_thread_join(&s3, t3);
apr_thread_join(&s4, t4);

ABTS_INT_EQUAL(tc, APR_SUCCESS, s1);
ABTS_INT_EQUAL(tc, APR_SUCCESS, s2);
ABTS_INT_EQUAL(tc, APR_SUCCESS, s3);
ABTS_INT_EQUAL(tc, APR_SUCCESS, s4);
}

static void test_remove(abts_case *tc, void *data)
{
apr_status_t rv;
Expand Down Expand Up @@ -94,6 +149,72 @@ static void test_removeall(abts_case *tc, void *data)

rv = apr_dir_remove("data/one", p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);

rv = apr_dir_remove("data/prll/one/thwo/three", p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);

rv = apr_dir_remove("data/prll/one/thwo", p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);

rv = apr_dir_remove("data/prll/one", p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);

rv = apr_dir_remove("data/prll/four/five/six/seven/eight", p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);

rv = apr_dir_remove("data/prll/four/five/six/seven", p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);

rv = apr_dir_remove("data/prll/four/five/six", p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);

rv = apr_dir_remove("data/prll/four/five", p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);

rv = apr_dir_remove("data/prll/four", p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);

rv = apr_dir_remove("data/prll/nine/ten", p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);

rv = apr_dir_remove("data/prll/nine", p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);

rv = apr_dir_remove("data/prll/11/12/13/14/15/16/17/18/19/20", p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);

rv = apr_dir_remove("data/prll/11/12/13/14/15/16/17/18/19", p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);

rv = apr_dir_remove("data/prll/11/12/13/14/15/16/17/18", p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);

rv = apr_dir_remove("data/prll/11/12/13/14/15/16/17", p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);

rv = apr_dir_remove("data/prll/11/12/13/14/15/16", p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);

rv = apr_dir_remove("data/prll/11/12/13/14/15", p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);

rv = apr_dir_remove("data/prll/11/12/13/14", p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);

rv = apr_dir_remove("data/prll/11/12/13", p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);

rv = apr_dir_remove("data/prll/11/12", p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);

rv = apr_dir_remove("data/prll/11", p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);

rv = apr_dir_remove("data/prll", p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);

rv = apr_dir_remove("data/fortytwo", p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
}

static void test_remove_notthere(abts_case *tc, void *data)
Expand Down Expand Up @@ -251,6 +372,7 @@ abts_suite *testdir(abts_suite *suite)

abts_run_test(suite, test_mkdir, NULL);
abts_run_test(suite, test_mkdir_recurs, NULL);
abts_run_test(suite, test_mkdir_recurs_parallel, NULL);
abts_run_test(suite, test_remove, NULL);
abts_run_test(suite, test_removeall_fail, NULL);
abts_run_test(suite, test_removeall, NULL);
Expand Down

0 comments on commit 1d7f658

Please sign in to comment.