From 1d7f658f0eabce29ca20c55a3530747167cf606f Mon Sep 17 00:00:00 2001 From: Branko Cibej Date: Tue, 21 Jan 2014 01:17:29 +0000 Subject: [PATCH] Fix timing bug in parallel apr_dir_make_recursive on Windows. * 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 --- file_io/win32/dir.c | 41 +++++++++------ test/testdir.c | 122 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 146 insertions(+), 17 deletions(-) diff --git a/file_io/win32/dir.c b/file_io/win32/dir.c index f6eeb87256a..5c30af8fdc3 100644 --- a/file_io/win32/dir.c +++ b/file_io/win32/dir.c @@ -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 */ } } @@ -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; } diff --git a/test/testdir.c b/test/testdir.c index 08ee7ec2c57..53972009581 100644 --- a/test/testdir.c +++ b/test/testdir.c @@ -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) @@ -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; @@ -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) @@ -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);