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

[Kieran Bingham]

Hi Laurent,

On 10/09/2019 10:40, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> 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?

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?


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
--
Kieran