Re: [PATCH 03/10] run-command: add an asynchronous parallel child processor

Stefan Beller <sbeller@google.com> writes:

Be nice and distribute the line evenly around "C". Same for thread 2 below.

The 'fn' feels really misnamed, especially when compared with the other fields. fn_task or something, perhaps.

Also I think we call a function type we define with a name that ends with _fn, e.g.

    typedef void (*show_commit_fn)(struct commit *, void *);
    void traverse_commit_list(struct rev_info *revs,
                              show_commit_fn show_commit,
                              show_object_fn show_object,
                              void *data)

So perhaps

	get_next_task_fn get_next_task;
        start_failure_fn start_failure;
        return_value_fn return_value;

or something like that.

What does slots[i] mean? Whatever explanation you would use as an answer to that question, I'd name the field after the key words used in the explanation. For example, if it means "children[i] is in use with a process", then the code would be a lot happier if the field is called in_use[] or something.

But do not just rename the field yet...

struct pollfd needs to be a contiguous array of nr_processes elements because that is what poll(2) takes, but other per-child fields would be easier to grasp if you did it like so:

	struct parallel_processes {
        	...
                struct {
                	int in_use;
                        struct child_process child;
                        struct strbuf err;
                        ... /* maybe other per-child field later */
		} *children;
		...
	}

A strbuf that holds "finished_children"? Does it hold "what finished_children said"?

We care about good naming because clearly named variables and fields make the code easier to read.

I would probably have called this "set_nonblocking()".

The first line of "warning(" is indented one level too deep.

Wouldn't it be easier to follow if you did

	fn(...)
	{
                if (flags < 0)
                        warn();
                else if (fcntrl() < 0)
                        warn();
	}

Drop this comment and replace it with a blank line.

Remove "&& " and add " &&" at the end of the previous line.

The comment does not help us at all. if (...) break; tells as much, and it does not tell us what it means that slot[] being empty at all.

		for (...)
			if (!pp->children[i].in_use)
				break;

would be a lot easier to follow without any comment.

This use pattern (and the explanation in run-command.h) suggests that the name of the field should be s/fn/more_task/; or something along that line (and flip the return value, i.e. more_task() returns yes if it did grab another task, no if there is no more task).

Can fn_err be NULL here? Shouldn't it (to give a default behaviour to lazy or bog-standard callers)?

Have a blank line here between decls and the first statement.

Give a symbolic constant for this 100ms, e.g. OUTPUT_POLL_INTERVAL or something.

Shouldn't this be more like

	while ((i = poll()) < 0)  {
        	if (errno == EINTR)
                	continue;
		cleanup;
		die;
	}

The caller after all was willing to wait for some time, and we were interrupted before that time came.

Even if we own the output channel, we may not have read anything yet---poll() may have said that pfd[i] is not ready, or read_nonblock() may have returned EWOULDBLOCK. Perhaps check not just that i owns the output but err[i].len is not zero?

I think output should be done outside the loop and make the comment before the loop match what the loop actually does.

	/* Buffer output from all pipes. */
	for (i = 0; ...) {
		if (pp->children[i].in_use && (pp->pfd[i].revents & POLLIN))
			strbuf_read_nonblock();
	}

	/* Drain the output from the owner of the output channel */
	if (pp->children[pp->output_owner].in_use &&
	    pp->children[pp->output_owner].err.len) {
	    	fputs(...);
                strbuf_reset(...);
	}

Drop this comment and replace it with a blank line.

Do we need that comment?

Can we get EINTR here (we are passing WNOHANG above)?

What should happen when we get an error not listed here? 'i' is left as initialized to 0 and we do strbuf_read_nonblock() for the first child (which may not even be running)?

You can sweep this bug under the rug by returning here, but I suspect that you would just want

	if (pid < 0)
		die_errno();

And that would allow you to dedent the else clause below.

Hmm, you are relying on the fact that a valid pid can never be 0, so you can just use pp->children[i].child.pid to see if a "slot" is occupied without even using pp->slots[] (or pp->children[i].in_use).

This is to read leftover output?

As discussed elsewhere, read_nonblock() will have to have "read some, not necessarily to the end" semantics to serve the caller in run_processes_parallel_buffer_stderr(), so you'd need a loop around it here to read until you see EOF.

Or you may be able to just call strbuf_read() and the function may do the right thing to read things through to the EOF. It depends on how you redo the patch [2/10].

You are clobbering errno by calling determine_return_value() but you do not use the returned value anywhere. Intended? Or should that be given to fn_exit() for error reporting?

Mental note: here the "slot" is cleared for the child 'i'.

OK, so the idea is that pp->child[i].err holds the entire output for any process that does not own the output channel until it dies, and they are appended to pp->finished_children. That suggests that the name of the "finished" field should have "output" somewhere in it.

Mental note: this side of if/else is what happens to the process that used to own the output channel.

... and it just flushes the final part of the output.

If there is any, that is.

... and then picks a new owner of the output channel.

Up to this point it looks sensible.

I do not think these two lines need to be here, especially if you follow the above advice of separating buffering and draining.

The former is true as long as more_task() says there may be more. The latter is true as long as we have something already running.

In either case, we should keep collecting and spawning as needed.

But calling start_new() unconditionally feels sloppy. It should at least be something like

	if (pp.nr_processes < pp.max_processes &&
	    !pp.all_task_started)
		start_new_process()

no?

So not only fn_err, but fn_exit is optional. You'd need to update the code above to allow these.

It was a fun read. There were tons of niggles, but I didn't see anything fundamentally unsalvageable.

Thanks.