You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2021/04/10 15:35:23 UTC
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&view=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&r1=1888588&r2=1888589&view=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&view=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 <stdio.h>
+#include <string.h>
+#include <apr_pools.h>
+
+#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,
+ noop_thead_context_constructor, NULL,
+ noop_cancel_func, NULL, pool, pool));
+ SVN_ERR(svn_task__run(2,
+ noop_process_func, NULL,
+ noop_output_func, NULL,
+ noop_thead_context_constructor, NULL,
+ noop_cancel_func, NULL, pool, pool));
+ return SVN_NO_ERROR;
+}
+
+static svn_error_t *
+counter_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)
+{
+ 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));
+ *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;
+
+ 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,
+ void *result,
+ void *output_baton,
+ svn_cancel_func_t cancel_func,
+ void *cancel_baton,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
+{
+ apr_int64_t *result_p = result;
+ apr_int64_t *output_p = output_baton;
+
+ if (result_p)
+ *output_p += *result_p;
+
+ if (cancel_func)
+ SVN_ERR(cancel_func(cancel_baton));
+
+ return SVN_NO_ERROR;
+}
+
+static svn_error_t *
+test_counting(apr_pool_t *pool)
+{
+ apr_int64_t start = 1000000;
+ apr_int64_t result = 0;
+ SVN_ERR(svn_task__run(1, counter_func, &start, sum_func, &result,
+ NULL, NULL, NULL, NULL, pool, pool));
+ SVN_TEST_ASSERT(result == start);
+
+ result = 0;
+ SVN_ERR(svn_task__run(4, counter_func, &start, sum_func, &result,
+ NULL, NULL, NULL, NULL, pool, pool));
+ SVN_TEST_ASSERT(result == start);
+
+ return SVN_NO_ERROR;
+}
+
+static svn_error_t *
+cancel_at_10k(void *baton)
+{
+ if (*(apr_int64_t*)baton == 10000)
+ return svn_error_create(SVN_ERR_CANCELLED, NULL, NULL);
+
+ return SVN_NO_ERROR;
+}
+
+static svn_error_t *
+test_cancellation(apr_pool_t *pool)
+{
+ apr_int64_t start = 1000000;
+ apr_int64_t result = 0;
+ SVN_TEST_ASSERT_ERROR(svn_task__run(1, counter_func, &start, sum_func, &result,
+ NULL, NULL, cancel_at_10k, &result,
+ pool, pool),
+ SVN_ERR_CANCELLED);
+ SVN_TEST_ASSERT(result == 10000);
+
+ result = 0;
+ SVN_TEST_ASSERT_ERROR(svn_task__run(8, counter_func, &start, sum_func, &result,
+ NULL, NULL, cancel_at_10k, &result,
+ pool, pool),
+ SVN_ERR_CANCELLED);
+ SVN_TEST_ASSERT(result == 10000);
+
+ return SVN_NO_ERROR;
+}
+
+/* An array of all test functions */
+
+static int max_threads = 1;
+
+static struct svn_test_descriptor_t test_funcs[] =
+ {
+ SVN_TEST_NULL,
+ SVN_TEST_PASS2(test_null_task,
+ "null-task"),
+ SVN_TEST_PASS2(test_noop_task,
+ "no-op task"),
+ SVN_TEST_PASS2(test_counting,
+ "concurrent counting"),
+ SVN_TEST_PASS2(test_cancellation,
+ "cancelling tasks"),
+ SVN_TEST_NULL
+ };
+
+SVN_TEST_MAIN
Propchange: subversion/trunk/subversion/tests/libsvn_subr/task-test.c
------------------------------------------------------------------------------
svn:eol-style = native
Re: svn commit: r1888589 - in /subversion/trunk: build.conf subversion/tests/libsvn_subr/task-test.c
Posted by Nathan Hartman <ha...@gmail.com>.
On Wed, Apr 21, 2021 at 11:58 PM Daniel Shahaf <d....@daniel.shahaf.name> 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
Posted by Nathan Hartman <ha...@gmail.com>.
On Wed, Apr 21, 2021 at 11:58 PM Daniel Shahaf <d....@daniel.shahaf.name> 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
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
stefan2@apache.org wrote on Sat, Apr 10, 2021 at 15:35:23 -0000:
> +++ 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 = 1000000;
⋮
> + result = 0;
> + SVN_TEST_ASSERT_ERROR(svn_task__run(8, counter_func, &start, sum_func, &result,
> + NULL, NULL, cancel_at_10k, &result,
> + pool, pool),
> + SVN_ERR_CANCELLED);
> + SVN_TEST_ASSERT(result == 10000);
Does the API actually guarantee that «result» will be equal to 10000?
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
Re: svn commit: r1888589 - in /subversion/trunk: build.conf
subversion/tests/libsvn_subr/task-test.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
stefan2@apache.org wrote on Sat, Apr 10, 2021 at 15:35:23 -0000:
> +++ 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 = 1000000;
⋮
> + result = 0;
> + SVN_TEST_ASSERT_ERROR(svn_task__run(8, counter_func, &start, sum_func, &result,
> + NULL, NULL, cancel_at_10k, &result,
> + pool, pool),
> + SVN_ERR_CANCELLED);
> + SVN_TEST_ASSERT(result == 10000);
Does the API actually guarantee that «result» will be equal to 10000?
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