| Message ID | 20190910090418.30502-2-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | ee35abb7c7e0ba26d6453fb5e1c3b79e596c9fd0 |
| Headers | show |
| Series |
|
| Related | show |
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. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Register the handler before the process is launched. > > Signed-off-by: Kieran Bingham <kieran.bingham@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())
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@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@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()) >
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@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@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())
Hi Laurent, On 10/09/2019 11:08, Laurent Pinchart wrote: > Hi Kieran, > > On Tue, Sep 10, 2019 at 11:02:56AM +0100, Kieran Bingham wrote: >> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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 :-) Is this rewrite acceptable to you ? test: process: Connect signal before launching process The procFinished event handler is registered after the process is started. This doesn't actually create any race, as the finished signal is emitted after a SIGCHLD is caught and handled through the ProcessManager and processed by the event loop. To make it clear that resources are connected before the process runs, register the handler before the process is started. >>>> Signed-off-by: Kieran Bingham <kieran.bingham@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()) >
Hi Kieran, On Wed, Sep 11, 2019 at 09:19:16AM +0100, Kieran Bingham wrote: > On 10/09/2019 11:08, Laurent Pinchart wrote: > > On Tue, Sep 10, 2019 at 11:02:56AM +0100, Kieran Bingham wrote: > >> > >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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 :-) > > Is this rewrite acceptable to you ? > > test: process: Connect signal before launching process > > The procFinished event handler is registered after the process is > started. This doesn't actually create any race, as the finished signal > is emitted after a SIGCHLD is caught and handled through the > ProcessManager and processed by the event loop. > > To make it clear that resources are connected before the process runs, > register the handler before the process is started. I would say something along the lines of "However, to follow the best practice that resources should be acquired before performing an action, connect the finished signal before starting the process.". Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>>> Signed-off-by: Kieran Bingham <kieran.bingham@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())
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())
The procFinished event handler is registered after the process is started, leading to the opportunity of a missed race. Register the handler before the process is launched. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- test/process/process_test.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)