--fork behaving as single threaded

[luissantosHCIT]

The Problem

When --fork is invoked, storescp accepts the associations up to the limit defined in --max-associations and then proceeds to downloading subsequent associations one at a time as if --exec-sync or --single-process was called.

Looking into the codebase, I noticed that there is an attempt at cleaning at least one zombie process from the child list at every invocation of an external processing command. This leads to single-threaded processing of incoming associations.

  pid_t pid = fork();
  if( pid < 0 )     // in case fork failed, dump an error message
    OFLOG_ERROR(storescpLogger, "cannot execute command '" << cmd << "' (fork failed)");
  else if (pid > 0)
  {
    /* we are the parent process. If we are in fork mode or
     * in synchronous exec mode, wait for the child process to terminate
     * and then clean up the process to avoid interference with the
     * counter that counts the number of child process in the main process
     */
    cleanChildren(pid, opt_execSync || opt_forkMode);
  }
  else // in case we are the child process, execute the command etc.
  {
    // execute command through execl will terminate the child process.
    // Since we only have a single command string and not a list of arguments,
    // we 'emulate' a call to system() by passing the command to /bin/sh
    // which hopefully exists on all Posix systems.

    if (execl( "/bin/sh", "/bin/sh", "-c", cmd.c_str(), OFreinterpret_cast(char *, 0) ) < 0)
      OFLOG_ERROR(storescpLogger, "cannot execute /bin/sh");

    // if execl succeeds, this part will not get executed.
    // if execl fails, there is not much we can do except bailing out.
    abort();
  }

The current version of storescp already has a mechanism checking if we are under the --max-association context which can only be invoked with --fork. It also only waits for one of the children to clear up before moving forward.

    if (cmd.findOption("--max-associations"))
    {
      app.checkDependence("--max-associations", "--fork", opt_forkMode);
      app.checkValue(cmd.getValueAndCheckMin(opt_maxChildren, 1));
    }
    ....
    /* if the maximum number of child processes is active,
     * wait until at least one child terminates before
     * continuing to accept incoming associations
     */
    if (numChildren == opt_maxChildren)
    {
        OFLOG_INFO(storescpLogger, "Maximum number of associations reached, waiting for child process to terminate");
        while (numChildren == opt_maxChildren)
        {
            cleanChildren(-1, OFTrue);
        }
    }
#endif

    /* receive an association and acknowledge or reject it. If the association was */
    /* acknowledged, offer corresponding services and invoke one or more if required. */
    cond = acceptAssociation(net, asccfg, tlsOptions.secureConnectionRequested());

    /* remove zombie child processes, do not block */
    cleanChildren(-1, OFFalse);

    /* since storescp is usually terminated with SIGTERM or the like,
     * we write back an updated random seed after every association handled.
     */
    cond = tlsOptions.writeRandomSeed();

My Solution

My changes here are to remove the opt_forkMode option inclusion during the zombie cleanup check since we do not want to block execution. I also included the idea to wait on all opt_maxChildren batch before spawning the next batch of processors.

As far as I can tell, the changes were stable processing 5K+ associations in my laptop, which was less tolerant of resource exhaustion than other systems in which I have used dcmtk. I tested with and without --max-associations. I know your team has a robust testing suite so I shall wait to see if changes here are enough and sufficient.

If you believe a further tweak to the changes should be included, let me know.