On Wed, Mar 14, 2012 at 2:26 PM, Patrick Ohly <[email protected]> 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. > > _______________________________________________ SyncEvolution mailing list [email protected] http://lists.syncevolution.org/listinfo/syncevolution
