Re: svn commit: r1888589 - in /subversion/trunk: build.conf subversion/tests/libsvn_subr/task-test.c
On Wed, Apr 21, 2021 at 11:58 PM Daniel Shahaf wrote: > > + partial_result = apr_palloc(result_pool, sizeof(partial_result)); > > s/sizeof(foo)/sizeof(*foo)/, here and later in the function. > > I'm surprised that no one caught this in post-commit reviews. In fact, > I wonder whether people have reviewed this commit and missed the bug, or > didn't review this commit at all — the latter being a silent failure > mode of the commit-then-review paradigm. (More on this under separate > cover — currently on private@, but may move here later.) (snip) Regarding counter_func(): > I'm not sure I follow what's happening here, and there aren't any > comment breadcrumbs either. I cannot grok this function either. It needs either comments to explain the intent here, or the test should be changed entirely, because I think (from my vague partial understanding) that the test is flawed. However, before jumping to conclusions, I'd like to wait for Stefan^2 to enlighten us. Meanwhile, since these 5 allocations are obviously wrong, I fixed them in r1889114 to prevent them from being forgotten there indefinitely. > test_counting() passes even if I change the > RHS's of both assignments to «*partial_result» to random values. > (test_cancellation() does fail in that case.) It looks to me (again, based on a vague partial understanding) that the test execution will self-adjust for whatever you assign here, as long as you assign a non-negative integer. If you change the RHS to a negative value, the test will (I believe) run for a very long time, and if you assign -1 to both cases or 1 to the first and -1 to the second, I think it will run forever. Two additional (very minor) issues with r1888589: The log message makes no mention of the new file subversion/tests/libsvn_subr/task-test.c, and that file starts with a spurious blank line. (It also had some trailing whitespace on various lines, but I inadvertently removed those in r1889114, much to my chagrin.) Cheers, Nathan
Re: svn commit: r1888589 - in /subversion/trunk: build.conf subversion/tests/libsvn_subr/task-test.c
stef...@apache.org wrote on Sat, Apr 10, 2021 at 15:35:23 -: > +++ subversion/trunk/subversion/tests/libsvn_subr/task-test.c Sat Apr 10 > 15:35:23 2021 > @@ -0,0 +1,253 @@ > +static svn_error_t * > +counter_func(void **result, ⋮ > + apr_pool_t *scratch_pool) > +{ > + apr_int64_t value = *(apr_int64_t*)process_baton; > + > + apr_pool_t *sub_task_pool; > + apr_int64_t *partial_result; > + apr_int64_t *partial_baton; > + > + if (value > 1) > +{ > + partial_result = apr_palloc(result_pool, sizeof(partial_result)); s/sizeof(foo)/sizeof(*foo)/, here and later in the function. I'm surprised that no one caught this in post-commit reviews. In fact, I wonder whether people have reviewed this commit and missed the bug, or didn't review this commit at all — the latter being a silent failure mode of the commit-then-review paradigm. (More on this under separate cover — currently on private@, but may move here later.) > + *partial_result = 1; > + value -= *partial_result; > + > + sub_task_pool = svn_task__create_process_pool(task); > + > + partial_baton = apr_palloc(sub_task_pool, sizeof(partial_baton)); > + *partial_baton = MAX(1, value / 2); > + value -= *partial_baton; > + > + SVN_ERR(svn_task__add_similar(task, sub_task_pool, > +partial_result, partial_baton)); > +} > + > + if (cancel_func) > +SVN_ERR(cancel_func(cancel_baton)); > + > + if (value > 1) > +{ > + partial_result = apr_palloc(result_pool, sizeof(partial_result)); > + *partial_result = 1; > + value -= *partial_result; > + I'm not sure I follow what's happening here, and there aren't any comment breadcrumbs either. Why is cancel_func called between the two blocks rather than once at the start (or end), which is the more common pattern? Why is «partial_result» allocated and initialized in both blocks, rather than just once? Why is «value» decremented by 1 in both blocks, rather than just once? Which line of code is responsible for doing this recursive call's work? test_counting() passes even if I change the RHS's of both assignments to «*partial_result» to random values. (test_cancellation() does fail in that case.) Why is «value» decremented (below) by an integer expression that's equal to «value - 1», rather than simply being assigned «value = 1»? Please reduce variables' scope to block scope where possible. > + sub_task_pool = svn_task__create_process_pool(task); > + > + partial_baton = apr_palloc(sub_task_pool, sizeof(partial_baton)); > + *partial_baton = value - 1; > + value -= *partial_baton; > + SVN_ERR(svn_task__add_similar(task, sub_task_pool, > +partial_result, partial_baton)); > +} > + > + partial_result = apr_palloc(result_pool, sizeof(partial_result)); > + *partial_result = value; > + *result = partial_result; > + > + return SVN_NO_ERROR; > +} > + > +static svn_error_t * > +sum_func(svn_task__t *task, ⋮ > +{ > + apr_int64_t *result_p = result; > + apr_int64_t *output_p = output_baton; > + > + if (result_p) What's the purpose of this check? It should never be false. Should it be removed? A segfault is less likely to result in a false negative PASS. > +*output_p += *result_p; ⋮ > +} ⋮ > +static svn_error_t * > +test_cancellation(apr_pool_t *pool) > +{ > + apr_int64_t start = 100; ⋮ > + result = 0; > + SVN_TEST_ASSERT_ERROR(svn_task__run(8, counter_func, , sum_func, > , > + NULL, NULL, cancel_at_10k, , > + pool, pool), > +SVN_ERR_CANCELLED); > + SVN_TEST_ASSERT(result == 1); Does the API actually guarantee that «result» will be equal to 1? The docs say: * Errors are detected in strictly in post-order, with only the first one * being returned from the task runner. In particular, it may not be the * first error that occurs timewise but only the first one encountered when * walking the tree and checking the processing results for errors. Because * it takes some time to make the workers cancel all outstanding tasks, * additional errors may occur before and after the one that ultimately * gets reported. All those errors are being swallowed. > +SVN_TEST_MAIN By the way, the docstring of svn_task__add() says "the current tasks output function". That's missing an apostrophe, isn't it? Cheers, Daniel
svn commit: r1888589 - in /subversion/trunk: build.conf subversion/tests/libsvn_subr/task-test.c
Author: stefan2 Date: Sat Apr 10 15:35:23 2021 New Revision: 1888589 URL: http://svn.apache.org/viewvc?rev=1888589=rev Log: Add some basic tests for the new svn_task__t API. * build.conf (task-test): Define new binary. (__ALL_TESTS__): Register new test binary. Added: subversion/trunk/subversion/tests/libsvn_subr/task-test.c (with props) Modified: subversion/trunk/build.conf Modified: subversion/trunk/build.conf URL: http://svn.apache.org/viewvc/subversion/trunk/build.conf?rev=1888589=1888588=1888589=diff == --- subversion/trunk/build.conf (original) +++ subversion/trunk/build.conf Sat Apr 10 15:35:23 2021 @@ -1120,6 +1120,14 @@ sources = sqlite-test.c install = test libs = libsvn_test libsvn_subr apriconv apr +[task-test] +description = Test concurrent tasks +type = exe +path = subversion/tests/libsvn_subr +sources = task-test.c +install = test +libs = libsvn_test libsvn_subr apr + [time-test] description = Test time functions type = exe @@ -1578,7 +1586,7 @@ libs = __ALL__ repos-test authz-test dump-load-test checksum-test compat-test config-test hashdump-test mergeinfo-test opt-test packed-data-test path-test prefix-string-test - priority-queue-test root-pools-test stream-test + priority-queue-test root-pools-test stream-test task-test string-test time-test utf-test bit-array-test filesize-test error-test error-code-test cache-test spillbuf-test crypto-test revision-test Added: subversion/trunk/subversion/tests/libsvn_subr/task-test.c URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_subr/task-test.c?rev=1888589=auto == --- subversion/trunk/subversion/tests/libsvn_subr/task-test.c (added) +++ subversion/trunk/subversion/tests/libsvn_subr/task-test.c Sat Apr 10 15:35:23 2021 @@ -0,0 +1,253 @@ + +/* + * task-test.c: a collection of svn_task__* tests + * + * + *Licensed to the Apache Software Foundation (ASF) under one + *or more contributor license agreements. See the NOTICE file + *distributed with this work for additional information + *regarding copyright ownership. The ASF licenses this file + *to you under the Apache License, Version 2.0 (the + *"License"); you may not use this file except in compliance + *with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + *Unless required by applicable law or agreed to in writing, + *software distributed under the License is distributed on an + *"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + *KIND, either express or implied. See the License for the + *specific language governing permissions and limitations + *under the License. + * + */ + +/* + To add tests, look toward the bottom of this file. + +*/ + +#include +#include +#include + +#include "../svn_test.h" + +#include "svn_sorts.h" +#include "private/svn_atomic.h" +#include "private/svn_task.h" + +static svn_error_t * +test_null_task(apr_pool_t *pool) +{ + SVN_ERR(svn_task__run(1, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, +pool, pool)); + SVN_ERR(svn_task__run(2, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, +pool, pool)); + + return SVN_NO_ERROR; +} + +static svn_error_t * +noop_process_func(void **result, + svn_task__t *task, + void *thread_context, + void *process_baton, + svn_cancel_func_t cancel_func, + void *cancel_baton, + apr_pool_t *result_pool, + apr_pool_t *scratch_pool) +{ + *result = NULL; + return SVN_NO_ERROR; +} + +static svn_error_t * +noop_output_func(svn_task__t *task, + void *result, + void *output_baton, + svn_cancel_func_t cancel_func, + void *cancel_baton, + apr_pool_t *result_pool, + apr_pool_t *scratch_pool) +{ + return SVN_NO_ERROR; +} + +static svn_error_t * +noop_thead_context_constructor(void **thread_context, + void *baton, + apr_pool_t *result_pool, + apr_pool_t *scratch_pool) +{ + *thread_context = NULL; + return SVN_NO_ERROR; +} + +static svn_error_t * +noop_cancel_func(void *baton) +{ + return SVN_NO_ERROR; +} + +static svn_error_t * +test_noop_task(apr_pool_t *pool) +{ + SVN_ERR(svn_task__run(1, +noop_process_func, NULL, +noop_output_func, NULL, +