On Fri, Mar 16, 2012 at 1:52 PM, Patrick Ohly <patrick.ohly(a)intel.com> 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 <patrick.ohly(a)intel.com> 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
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
> > 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
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.