7:52 a.m.
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
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.
> 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.
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.
So we have a pretty complex logic now: allow multiple
ConnectionResources to be active, but prevent running multiple syncs.
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?
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.
--
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.