Review of task.c as of svn commit: r1888520 - in /subversion/trunk: build.conf subversion/libsvn_subr/task.c

[Daniel Shahaf <d....@daniel.shahaf.name>]

Good morning Stefan,

stefan2@apache.org wrote on Thu, Apr 08, 2021 at 14:35:52 -0000:
> Initial, single-theaded implementation of the svn_task__t API.

I've committed some minor fixes in r1888533.  Most of them are
straightforward but please double check that all the apostrophes are
where they should be.

Here's also a review of task.c as it stands now.

Index: subversion/include/private/svn_task.h
===================================================================
--- subversion/include/private/svn_task.h	(revision 1888527)
+++ subversion/include/private/svn_task.h	(working copy)
@@ -117,6 +117,9 @@ typedef svn_error_t *(*svn_task__process_func_t)(
  * All data that shall be returned by @a svn_task__run() needs to be
  * allocated from @a result_pool.
  *
+ * @note The lifetime of @a result is not guaranteed. If it is to be returned
+ * from svn_task__run(), it needs to be copied to @a result_pool.
+ *
  * @a cancel_func, @a cancel_baton and @a scratch_pool are the usual things.
  */
 typedef svn_error_t *(*svn_task__output_func_t)(
Index: subversion/libsvn_subr/task.c
===================================================================
--- subversion/libsvn_subr/task.c	(revision 1888533)
+++ subversion/libsvn_subr/task.c	(working copy)
@@ -139,7 +139,8 @@ struct svn_task__t
 
   /* Parent task.
    * NULL for the root node:
-   * (PARENT==NULL) == (*this==ROOT->TASK).
+   * (PARENT==NULL) == (this==ROOT->TASK).
+   * ### assert() that?
    */
   svn_task__t *parent;
 
@@ -177,6 +178,7 @@ struct svn_task__t
   /* Task state. */
 
   /* The callbacks to use. Never NULL. */
+  /* ### assert() that? */
   callbacks_t *callbacks;
 
   /* Process baton to pass into CALLBACKS->PROCESS_FUNC. */
@@ -186,7 +188,7 @@ struct svn_task__t
    * Sub-pool of ROOT->PROCESS_POOL.
    *
    * NULL, iff processing of this task has completed (sub-tasks may still
-   * need processing).  Used to check whether processing has been completed.
+   * need processing).
    */
   apr_pool_t *process_pool;
 
@@ -203,12 +205,13 @@ struct svn_task__t
 
 /* Return the index of the first immediate sub-task of TASK with a ready
  * sub-task in its respective sub-tree.  TASK must have at least one such
- * sub-task.
+ * proper sub-task.
  */
 static apr_size_t first_ready_sub_task_idx(const svn_task__t *task)
 {
   svn_task__t *sub_task = task->first_ready;
   assert(sub_task);
+  assert(sub_task != task);
 
   while (sub_task->parent != task)
     sub_task = sub_task->parent;
@@ -391,6 +394,8 @@ static svn_error_t *remove_task(svn_task__t *task)
  * mainly to prevent error leaks when terminating the task runner early.
  * It is better to have the standard pool cleanups to deal with memory
  * management in that case.
+ *
+ * ### Also resets FIRST_SUB
  */
 static void free_sub_tasks(svn_task__t *task)
 {
@@ -503,6 +508,7 @@ static void process(svn_task__t *task,
     {
       /* Depending on whether there is prior parent output, we may or 
        * may not have already an OUTPUT structure allocated for TASK. */
+      /* ### Aside: The names could be clearer.  As it is, output == task->output != output->output */
       output_t *output = ensure_output(task);
       output->error = callbacks->process_func(&output->output, task,
                                               thread_context,
@@ -555,7 +561,12 @@ static svn_error_t *output_processed(
       /* Post-order, i.e. dive into sub-tasks first.
        *
        * Note that the "background" processing for CURRENT has completed
-       * and only the output function may add no further sub-tasks. */
+       * and the "foreground" output function may add further sub-tasks. */
+      /* ### The 'if' branch processes current->first_sub->output->prior_parent_output —
+       * ### that's output produced by current — in this iteration, and only in
+       * ### the next iteration will handle current->first_sub's subtree's
+       * ### outputs.  That's not post-order, is it?
+       */
       if (current->first_sub)
         {
           current = current->first_sub;

HTH,

Daniel

Re: Review of task.c as of svn commit: r1888520 - in /subversion/trunk: build.conf subversion/libsvn_subr/task.c

[Stefan Sperling <st...@apache.org>]

On Sun, Apr 11, 2021 at 07:55:33AM +0200, Stefan Sperling wrote:
> Just adding another thing: It looks like task.c has undefined symbols
> during linking if APR is built without support for threads:

I see that this has been fixed now. Thank you!

Re: Review of task.c as of svn commit: r1888520 - in /subversion/trunk: build.conf subversion/libsvn_subr/task.c

[Stefan Sperling <st...@apache.org>]

On Thu, Apr 08, 2021 at 08:01:43PM +0000, Daniel Shahaf wrote:
> Good morning Stefan,
> 
> stefan2@apache.org wrote on Thu, Apr 08, 2021 at 14:35:52 -0000:
> > Initial, single-theaded implementation of the svn_task__t API.
> 
> I've committed some minor fixes in r1888533.  Most of them are
> straightforward but please double check that all the apostrophes are
> where they should be.
> 
> Here's also a review of task.c as it stands now.

Just adding another thing: It looks like task.c has undefined symbols
during linking if APR is built without support for threads:

../../subversion/libsvn_subr/.libs/libsvn_subr-1.so.0.0: undefined reference to `apr_thread_exit'
../../subversion/libsvn_subr/.libs/libsvn_subr-1.so.0.0: undefined reference to `apr_thread_join'
../../subversion/libsvn_subr/.libs/libsvn_subr-1.so.0.0: undefined reference to `apr_thread_create'
https://ci.apache.org/builders/svn-bb-openbsd/builds/673/steps/Build/logs/stdio

There's probably just a missing check for the APR_HAS_THREADS macro.

This build bot is running a non-threaded build in order to catch such issues.
In reality this is likely an non-issue for most people. But as long as we
intend to support the !APR_HAS_THREADS case our code should compile in
both cases.

I don't have time right now to fix this myself, sorry.

Cheers,
Stefan