[prev in list] [next in list] [prev in thread] [next in thread] 

List:       subversion-commits
Subject:    Re: svn commit: r1888589 - in /subversion/trunk: build.conf subversion/tests/libsvn_subr/task-test.c
From:       Daniel Shahaf <d.s () daniel ! shahaf ! name>
Date:       2021-04-22 3:57:54
Message-ID: 20210422035754.GA21625 () tarpaulin ! shahaf ! local2
[Download RAW message or body]

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
[prev in list] [next in list] [prev in thread] [next in thread]