Re: [PATCH v2] fetch: emphasize failure during submodule fetch

[Emily Shaffer <emilyshaffer@google.com>]

On Thu, Jan 16, 2020 at 01:55:26PM -0800, Emily Shaffer wrote:
> On Thu, Jan 16, 2020 at 10:23:58AM -0800, Junio C Hamano wrote:
> > Emily Shaffer <emilyshaffer@google.com> writes:
> > 
> > > @@ -1280,10 +1280,13 @@ struct submodule_parallel_fetch {
> > >  	/* Pending fetches by OIDs */
> > >  	struct fetch_task **oid_fetch_tasks;
> > >  	int oid_fetch_tasks_nr, oid_fetch_tasks_alloc;
> > > +
> > > +	struct strbuf submodules_with_errors;
> > > +	pthread_mutex_t submodule_errors_mutex;
> > 
> > Hmph, it is kind of surprising that we need a new mutex for this.
> > 
> > Isn't the task_finish handler, which is what accesses the
> > with_errors field this patch adds, called by pp_collect_finished()
> > one at a time, is it?
> 
> Hm. It is called by pp_collect_finished() one at a time, but while other
> processes may still be running. So I guess that is OK - spf might still
> be read by other tasks but this field of it won't be touched by anybody
> simultaneously. Ok, I'm convinced.
> 
> > It seems oid_fetch_tasks[] array is also a shared resource in this
> > structure among the parallel fetch tasks, but there is no protection
> > against simultaneous access to it.  Am I missing what makes the new
> > field different?  Somewhat puzzled...
> 
> I think it's similar. As I understand it, it looks something like this:
> 
>   loop forever:
>     can i start a new process?
>       get_next_task cb (blocking)
>       start work cb (nonblocking unless it failed to start)
>     process stderr in/out once (blocking)
>     is anybody done? (blocking)
>       task_finished cb (blocking) <- My change is in here
>         did fetch by ref fail? (blocking)
>           put fetch by OID onto the process list (blocking)
>     is everybody done?
>       break

Ah, as I look deeper I realize that it's a child process, not a thread,
so this code becomes even simpler to understand. I think then I don't
need to worry about thread safety at all here.

 - Emily