8:26 p.m.
W dniu 14 marca 2012 15:30 użytkownik Patrick Ohly
<patrick.ohly(a)intel.com> napisał:
Hello Krzesimir!
Hi!
Sorry for double post - forgot to use "Reply to all".
You added the add/remove_filter() methods to DBusConnectionPtr, with
the
comment "those additions will be needed for ForkExec ready message
handling" in the commit message.
The methods themselves are not documented. Can you explain a bit how
this is meant to work?
The history is that I needed to add a new signal to ForkExec, because
activation of DBus interface on child side was racing with using this
interface on parent side. Such thing happened very often for GIO GDBus
when creating Session - when sync-helper established a DBus connection
to server then server activated Session proxy interface, added Session
to active queue and immediately calls SetActive to mark SessionImpl as
active. It happened quite often that that call went to /dev/null,
because sync-helper didn't yet activated the SessionImpl's DBus
interface. That's why I added a ready signal which child (sync-helper)
can send to parent (server) to note that it is ready (DBus interface
is activated). And this works by sending a custom signal (thus
DBusMessagePtr::create_empty_signal(), some setters and
DBusConnectionPtr::send() are added) to parent and parent installs a
"one time" filter to look for such message (thus some getters in
DBusMessagePtr are added and that filter stuff in DBusConnectionPtr).
By "one time" I mean that the filter is installed as long as no such
signal appeared.
I stumbled over this when I added an error exception which prevents
adding new resources to a server which is already shutting down:
void Server::addResource(const Resource_t &resource, const
boost::function<void()> &callback)
{
==> if (m_shutdownRequested) {
==> // don't allow new resources, we cannot activate them
==> SE_THROW("server shutting down");
==> }
m_waitingResources.insert(resource);
checkQueue(callback);
}
It killed the syncevo-dbus-server because there was nothing catching the
exception. Regardless whether this is the right approach for solving the
shutdown problem, throwing and exception should never have that effect.
Further analysis showed that the call stack is (boost function levels
omitted):
#0 0x00007ffff092ab40 in __cxa_throw () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#1 0x0000000000d710cf in
SyncEvo::Server::addResource(boost::shared_ptr<SyncEvo::Resource> const&,
boost::function<void ()> const&) (this=0x7fffffffdb70, resource=...,
callback=...)
at /home/pohly/syncevolution/syncevolution/src/dbus/server/server.cpp:447
#2 0x0000000000d7179e in SyncEvo::Server::startSessionCb (this=0x7fffffffdb70,
client=..., resource=...,
result=...) at /home/pohly/syncevolution/syncevolution/src/dbus/server/server.cpp:190
...
#8 0x0000000000dc786f in
SyncEvo::SessionResource::onSessionReady(boost::function<void
(boost::shared_ptr<SyncEvo::SessionResource> const&)> const&)
(this=0x7fffe402d430, callback=...)
at /home/pohly/syncevolution/syncevolution/src/dbus/server/session-resource.cpp:403
...
#23 0x0000000000fc5c08 in SyncEvo::ForkExecParent::connectionFilter (this=0x7fffe402cf30,
conn=..., message=...)
at /home/pohly/syncevolution/syncevolution/src/syncevo/ForkExec.cpp:219
...
#29 0x0000000000f0389d in GDBusCXX::filter_cb (conn=0x1a7bdb0, message=0x1a928f0,
user_data=0x7fffe4033a80)
at /home/pohly/syncevolution/syncevolution/src/gdbusxx/gdbus-cxx-bridge.cpp:60
#30 0x00007ffff177dab8 in on_worker_message_received (worker=<optimized out>,
message=0x1a928f0,
user_data=0x1a7bdb0) at /tmp/buildd/glib2.0-2.30.2/./gio/gdbusconnection.c:2250
...
#42 0x00007ffff0be27e6 in g_thread_create_proxy (data=0x1a84460) at
/tmp/buildd/glib2.0-2.30.2/./glib/gthread.c:1962
#43 0x00007ffff671eb50 in start_thread (arg=<optimized out>) at
pthread_create.c:304
#44 0x00007ffff012b90d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:112
#45 0x0000000000000000 in ?? ()
Two observations:
1. GDBusCXX::filter_cb() is a callback for C code, and thus must
catch all exceptions in the code that it calls, then translate
them into some kind of C error return code (or whatever the
right error handling is).
Right, there should be an exception catching clause. Not sure about
right error handling though. Probably an error have to be signaled to
main thread (with g_idle_add?).
2. g_dbus_connection_add_filter() says that "filters are run
in a
dedicated message handling thread so they can't block and,
generally, can't do anything but signal a worker thread". These
are severe restrictions that aren't documented anywhere in the
add_filter() method, nor does our code conform to these
restrictions. For example, it calls the logging system, which is
not thread-safe.
I think we need a better solution for the problem. FWIW, the main thread
was idling at the time:
Thread 1 (Thread 0x7ffff7faf960 (LWP 25268)):
#0 0x00007ffff0120cc3 in *__GI___poll (fds=<optimized out>, nfds=<optimized
out>, timeout=10)
at ../sysdeps/unix/sysv/linux/poll.c:87
#1 0x00007ffff0bbd5d8 in g_main_context_poll (n_fds=4, fds=0x7fffe40329c0, timeout=10,
context=0x1a75780,
priority=<optimized out>) at /tmp/buildd/glib2.0-2.30.2/./glib/gmain.c:3391
#2 g_main_context_iterate (context=0x1a75780, block=<optimized out>, dispatch=1,
self=<optimized out>)
at /tmp/buildd/glib2.0-2.30.2/./glib/gmain.c:3071
#3 0x00007ffff0bbde02 in g_main_loop_run (loop=0x1a75870) at
/tmp/buildd/glib2.0-2.30.2/./glib/gmain.c:3284
#4 0x0000000000d75661 in SyncEvo::Server::run (this=0x7fffffffdb70)
at /home/pohly/syncevolution/syncevolution/src/dbus/server/server.cpp:398
#5 0x0000000000d6b6c4 in main (argc=1, argv=0x7fffffffe158, envp=0x7fffffffe168)
at /home/pohly/syncevolution/syncevolution/src/dbus/server/main.cpp:141
Perhaps the add_filter callback should be limited to the bare minimum of
functionality and do nothing more than waking up the main thread, to
finish whatever work is needed in ForkExec startup?
That is really my bad. I should probably just use g_idle_add, so the
callback will happen in main thread.
--
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.