[libcamera-devel] [PATCH 1/3] test: process: Fix forking race

[Laurent Pinchart]

Hi Kieran,

On Tue, Sep 10, 2019 at 11:02:56AM +0100, Kieran Bingham wrote:
> On 10/09/2019 10:40, Laurent Pinchart wrote:
> > On Tue, Sep 10, 2019 at 10:04:16AM +0100, Kieran Bingham wrote:
> >> The procFinished event handler is registered after the process is
> >> started, leading to the opportunity of a missed race.
> > 
> > That shouldn't be the case, as the procFinished signal is emitted from
> > the event loop. Still, connecting the signal before starting the process
> > is a good practice, so this patch is fine. You may want to mention in
> > the commit message that there's no actual race though.
> 
> Aha, OK so I misunderstood the code.
> 
> Do the events get 'cached' and stored then ? If a slot is not connected,
> can we accumulate an unlimited amount of events?

It's not about signals/slots, it's about listening to events. The
finished signal is emitted from Process::died(), called by
ProcessManager::sighandler(), itself a slot connected to the activated
signal of the sigEvent_ event notifier. The event notifier listens for
read events on the pipe used to communicate between the unix signal
handler (sigact() in process.cpp). Thus, when SIGCHLD is caught, we
write to the pipe, and the notification on the other side goes through
the event notifier, which is processed by the event loop.

> So if we connect a signal/slot at any arbitrary point in time later -
> will we receive all of the historical calls? Or just the latest?

Signals are not buffered, we keep no history.

> I've confirmed there is no race here by putting an arbitrary 5 second
> sleep after calling .start(), and before connecting the signal and it
> still works.
> 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> Thanks, I'll update the commit message here as there is indeed no actual
> race.
> 
> But it does make me feel a lot better to hook up the dependencies first :-)
> 
> >> Register the handler before the process is launched.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >>  test/process/process_test.cpp | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp
> >> index d264555e545f..701f156b5053 100644
> >> --- a/test/process/process_test.cpp
> >> +++ b/test/process/process_test.cpp
> >> @@ -47,12 +47,13 @@ protected:
> >>  		int exitCode = 42;
> >>  		vector<std::string> args;
> >>  		args.push_back(to_string(exitCode));
> >> +		proc_.finished.connect(this, &ProcessTest::procFinished);
> >> +
> >>  		int ret = proc_.start("/proc/self/exe", args);
> >>  		if (ret) {
> >>  			cerr << "failed to start process" << endl;
> >>  			return TestFail;
> >>  		}
> >> -		proc_.finished.connect(this, &ProcessTest::procFinished);
> >>  
> >>  		timeout.start(100);
> >>  		while (timeout.isRunning())

-- 
Regards,

Laurent Pinchart