2012/3/15 Patrick Ohly <[email protected]>:
> More observations/questions.
>
> ConnectionResource and SessionResource share a lot of code: init(),
> onQuit(), onFailure(), ... - basically the whole startup of the helper
> seems to have been copied. Shouldn't that common logic be in the
> Resource base class?
Maybe some of the code could be common, but not that whole. Proxy
handling is different for Session and Connection. Different
environment variables are used. onQuit and onFailure can be easily
moved. I will do it, but maybe bit later when more pressing issues are
solved.
>
> I also wonder about object ownership while the resource is still in the
> process of getting constructed.
>
>
> void SessionResource::createSessionResource(const Callback_t &callback,
> Server &server,
> const std::string &peerDeviceID,
> const std::string &configName,
> const std::string &session,
> const std::vector<std::string>
> &flags)
> {
> std::auto_ptr<SessionResource> resource(new SessionResource(server,
> peerDeviceID,
> configName,
> session,
> flags));
>
> resource->init(callback);
> // init did not throw an exception, so we guess that child was
> // spawned successfully. thus we release the auto_ptr, so it will
> // not delete the resource.
> resource.release();
> }
>
> "we guess" is not encouraging when it comes to memory ownership ;-} Do
> we know for sure? If createSessionResource() transfers owership to
> init(), then it should do so explicitly.
Ah, well, exception was not thrown at this point so I stated that all
went fine. The situation when resource could leak would be when there
was some failure during DBus communication between sync-helper and
server e.g. sync-helper crashed. I know - it is still ugly, so I will
change that it will return a shared_ptr to Session, but it will have
to be documented that such Session is not very usable at this point.
To use such Session (or Connection) server has to wait until callback
is called.
> Next, SessionResource::init(). It connects the resource to ForkExec
> signals, using a plain pointer to the resource, without the
> boost::signal2 track mechanism that I had suggested.
With creation of shared_ptr above it should be easy to solve now.
> Who owns the SessionResource at that point? Who is going to free it when
> something goes wrong before (eventually) addResource() is called? As far
> as I can tell, the instance will simply leak.
>
> Another situation is when the D-Bus client quits while its requested
> session still gets constructed. The Client instance of the client will
> get destructed. But the SessionResource was not yet connected to it, so
> the construction cannot be aborted by Server.
>
> 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).
>
> --
> 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
