On Fri, Mar 16, 2012 at 1:52 PM, Patrick Ohly <[email protected]> wrote: > On Fri, 2012-03-16 at 13:00 +0100, Chris Kühl wrote: >> On Fri, Mar 16, 2012 at 11:54 AM, Patrick Ohly <[email protected]> >> wrote: >> > On Fri, 2012-03-16 at 11:33 +0100, Patrick Ohly wrote: >> >> On Thu, 2012-03-15 at 14:43 +0100, Patrick Ohly wrote: >> >> > I still think resources should be added to m_waitingResources before >> >> > construction is complete. Activating the object and notifying the >> >> > caller >> >> > can wait until the resource is really usable (= helper ready). >> >> >> >> And one more observation: running the helper should get delayed until it >> >> is really needed, which probably means until the resource is ready to >> >> become active. >> >> >> >> Imagine having 100 pending session requests. An entry in >> >> m_waitingResources is cheap enough to handle that (admittedly unlikely) >> >> case, but combine that with 100 syncevo-dbus-helper processes and we >> >> have a problem. >> > >> > Speaking of more than one entry in m_waitingResources: does the current >> > PriorityCompare really implement a strict weak ordering, as required by >> > std::set, the storage used for m_waitingResource? >> > >> > I don't think it does, because two different resources with the same >> > priority will return false regardless which way you compare them, and >> > that despite not being equivalent. I bet that as a result, you currently >> > cannot store two resources with the same priority in m_waitingResource. >> > Even if you could, what would be the relative order? Everything else >> > being equal, we want first-come-first-serve. >> > >> >> oops, you're right. That should have been a multiset. >> >> Here is a small test I'd written to make sure the ordering was correct. > > ... but is it guaranteed to be correct, or does it just happen to work > as intended for your current implementation of std::multiset? I don't > see anything in the SGI STL documentation that says that a multiset > preserves the order in which elements are added. > > The Linux man page says "Iteration is done in ascending order according > to the keys." That leaves it undefined what the order is for equal keys. > >> > Another concern is that the outcome of PriorityCompare depends on being >> > able to lock shared pointers. If a resource goes away, then the >> > invariants for the set might get violated (comparison no longer returns >> > the same results as before). I don't want to find out how std::set deals >> > with that... >> > >> >> I believe I'm handling the case where the weak pointer goes away >> already. > > Your code does, but does std::multiset? It's data structure is based on > the result of the comparison between two elements. If that comparison > suddenly returns different results, some invariants inside std::multiset > get violated. All hell might break loose. >
You're absolutely right. Item 22 in "Effective STL" explains just this situation. Thanks for catching and explaining that. >> > In a nutshell, I think we need to go back to the original std::list and >> > its brute-force insertion sort. Agreed? >> > >> >> I found it a bit nicer to use the comparison class but if you're not >> convinced feel free to revert that. > > It's nicer, but I am not convinced that it is correct. And you're right. > > Another aspect is that m_waitingResources now mixes connections and > sessions. That wasn't the case before. A connection created a session > which then got scheduled, instead of being scheduled like one. There is > a reason for that: suppose there is a running connection for client A. > The client gives up on that connection and starts anew. Is that new > connection allowed to start? If not, it will be blocked by the stale > connection that it is meant to replace. > Like I said the Connection code is incomplete and this is because I wasn't convinced I was taking the right approach. In hindsight I would have started a connection immediately. Once the session needed to be created by the Connection the helper would create a Session object, set up the one-to-one dbus interface and wait for the Server to create a SessionResource then place it in the queue. At this point we'd be in the same situation as if only a Session had been requested. > So we have a pretty complex logic now: allow multiple > ConnectionResources to be active, but prevent running multiple syncs. > The above scheme would remove the need to have the wait queue and active resource list hold anything other than Sessions. > Speaking of running a sync with a ConnectionResource: where is the > SessionResource that exposes the sync to D-Bus clients? The old > Connection class had > boost::shared_ptr<Session> m_session > > That pointer still exists in the helper, but the counterpart in > syncevo-dbus-server is missing. Have you thought about this? > Again, the above scheme would lend itself to using m_session easily. > I'm wondering whether the old approach would be easier: > * keep the Connection logic inside syncevo-dbus-server > * schedule a normal SessionResource there > * use it once it is ready > > One downside of this is that Connection needs to parse the incoming > message, for which it must instantiate libsynthesis. It's linked to > anyway via libsyncevolution, but not used elsewhere, so long term we > could get rid of it in syncevo-dbus-server by refactoring the libs if it > wasn't for this one aspect. > I believe the scheme I proposed above would lead itself to achieving this goal. Cheers, Chris _______________________________________________ SyncEvolution mailing list [email protected] http://lists.syncevolution.org/listinfo/syncevolution
