Re: [SyncEvolution] shutdown request handling + fork/exec dbus-server
On Wed, Mar 14, 2012 at 2:26 PM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
Hello Chris!
Your patch removes the Server::m_shutdownSession and replaces it with a
boolean m_shutdownRequested + a timeout that is set once in
Server::fileModified().
The comment in server.h still refers to m_shutdownSession and explains
that its purpose was to prevent other sessions from running. How is that
handled now?
That is still the intent but now that you're pointing it out I see
that the implementation is not complete.
More specifically, consider the following sequence of events:
1. files are modified and the quiesence timeout is started
2. a client connects, starts a session and syncs
3. the timeout triggers, causing the server to shut down
If it works as designed (we don't have tests for it, do we?), then the
syncevo-dbus-helper will continue to run and complete the session. But
the client will see an unexpected server disconnect and won't be able to
monitor progress of the still running session, because the running
helper isn't connected to any syncevo-dbus-server instance.
It could even be worse: with bad timing, an obsolete syncevo-dbus-helper
might get started which then fails because it doesn't match the
installed files.
IMHO adding any kind of resource should be prevented while the
m_shutdownRequested flag is set. Returning an error message to the
client would be okay
Yes, and this is how it should work. I forgot to guard against this case.
Do you agree with this analysis or did I miss something?
Yes. That all sounds spot on.
Cheers,
Chris
Don't bother changing code, I'm just trying to clarify the
existing one.
--
Best Regards, Patrick Ohly
The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.