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@xxxxxxxxxx> 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
