• Stephan Bergmann's avatar
    Avoid race between DbusIpcThread::close and DbusIpcThread::execute · 38081c08
    Stephan Bergmann yazdı
    ...that caused Flatpak'ed LO to sometimes not terminate properly after
    243d743d "solenv/flatpak-manifest.in:
    incorporate upstream sandboxing improvements" had removed --socket=session-bus
    and thus introduced flatpak-dbus-proxy into the mix:
    
    > Oct 24 15:25:16 <sberg_> I'm not sure who's at fault there; on the LO side I
    >  have a thread processing incoming dbus requests (DbusIpcThread::execute,
    >  <https://opengrok.libreoffice.org/xref/core/desktop/source/app/officeipcthread.cxx#552>);
    >  that thread is typically waiting within dbus_connection_read_write_dispatch
    >  for new messages; if LO wants to close, it needs to get that thread out of
    >  dbus_connection_read_write_dispatch, and the only way I found back then is
    >  what's in DbusIpcThread::close (just following DbusIpcThread::execute), with
    >  a big "this apparently needs a more DBus-y design anyway" testament to my
    >  cluelesness re dbus; it calls dbus_bus_get_private to connect to itself, and
    >  send a Close message that'll cause DbusIpcThread::execute to fall out of the
    >  loop; the dbus_bus_get_private leads to a flatpak_proxy_incoming in the
    >  proxy, which returns TRUE, and after that I see no further activity in the
    >  proxy (but not sure what functions I should all put breakpoints on), and the
    >  LO side is hung in dbus_bus_release_name a few lines futher down
    > Oct 24 15:26:20 <alexlarsson> sberg_: i ran this:
    >  https://github.com/sgh/dbus-examples/blob/master/dbus-signal.c
    > Oct 24 15:26:27 <alexlarsson> sberg_: which calls dbus_bus_release_name
    > Oct 24 15:26:30 <alexlarsson> and it seems to work for me
    > Oct 24 15:26:39 <alexlarsson> If i --own-name=org.DBusTest.SignalTest
    > Oct 24 15:27:50 <sberg_> so maybe it's related to my unorthodox way of trying
    >  to quit the dbus loop there;  if anybody with a clue about dbus would be
    >  willing to help me with the above, I'd be very grateful :)
    > Oct 24 15:28:06 <alexlarsson> I don't see how the proxy should affect it tho
    > Oct 24 15:28:11 <alexlarsson> other than maybe timing?
    > Oct 24 15:29:06 <sberg_> maybe; I think I've seen things actually succeed once
    >  when I stepped through the proxy somewhat manually
    > Oct 24 15:31:01 <alexlarsson> sberg_: eeeentresting
    > Oct 24 15:31:07 <alexlarsson> sberg_: so, it does an own-call
    > Oct 24 15:31:13 <alexlarsson> sberg_: maybe that is what breaks?
    > Oct 24 15:31:33 <sberg_> yeah, that's my dumb speculation
    > Oct 24 15:34:38 <alexlarsson> So, you send the close from another thread, not
    >  waiting for the reply
    > Oct 24 15:34:56 <alexlarsson> then you start working on the main connection
    >  immediately
    > Oct 24 15:35:02 <alexlarsson> shouldn
    > Oct 24 15:35:25 <alexlarsson> shouldn't you grab the mutex after that?
    > Oct 24 15:36:35 <alexlarsson> This whole thing smells racy
    > Oct 24 15:38:18 <alexlarsson> My guess is that with regular dbus, the flush
    >  call will schedule dbus daemon which sends back the message, waking up the
    >  other thread before continuing
    > Oct 24 15:38:22 <sberg_> alexlarsson, ah, yeah, I see; apparently didn't
    >  occur to me back then that I mustn't call dbus_bus_release_name when the
    >  execute loop may still be busy
    > Oct 24 15:38:25 <alexlarsson> But now you have two context switches
    > Oct 24 15:38:37 <alexlarsson> so we continue before we get the reply
    > Oct 24 15:38:44 <alexlarsson> eh, not reply
    > Oct 24 15:38:46 <alexlarsson> the close message
    > Oct 24 15:38:52 <alexlarsson> and then you race
    > Oct 24 15:39:11 <alexlarsson> Honestly i don't remember the exact rules for
    >  dbus threadedness
    > Oct 24 15:39:58 <sberg_> ...so looks like something that can be corrected on
    >  the LO side after all; I'll give it a try
    > Oct 24 15:40:17 <alexlarsson> you're calling dbus_threads_init_default(), so
    >  it should be nominally threadsafe
    > Oct 24 15:40:29 <alexlarsson> still, I imagine it can still deadlock
    > Oct 24 15:43:12 <alexlarsson> sberg_: i still don't see the exact deadlock
    >  though
    > Oct 24 15:43:32 <alexlarsson> i imagine working on the separate private
    >  connection is threadsafe
    > Oct 24 15:43:35 <alexlarsson> and we flush that
    > Oct 24 15:44:08 <alexlarsson> then we call dbus_bus_release_name, which i
    >  imagine would block if the dbus thread is in read_write_dispatch
    > Oct 24 15:44:35 <alexlarsson> The question is, why is the close message not
    >  delivered?
    > Oct 24 15:44:44 <alexlarsson> I mean, we flushed it...
    > Oct 24 15:45:48 <alexlarsson> sberg_: oh, i think i know
    > Oct 24 15:45:53 <alexlarsson> sberg_: release_name probably sent some message,
    >  but then the other thread pop:ed the reply
    > Oct 24 15:46:04 <alexlarsson> sberg_: queue very long wait
    > Oct 24 15:51:53 <sberg_> alexlarsson, yeah, sounds plausible; thanks!
    > Oct 24 15:52:14 <sberg_> (now that I want to reproduce it, closing succeeds
    >  cleanly every time I try, of course...)
    > Oct 24 15:54:48 <alexlarsson> sberg_: obviously
    > Oct 24 15:55:30 <alexlarsson> sberg_: that just makes the race theory more
    >  valid tho
    > Oct 24 15:57:31 <sberg_> alexlarsson, yup, after stopping the unrelated
    >  background LO build that had put the machine under full load, I see it hang
    >  again in release_name, while the execute loop is still happily running too;
    >  that nicely confirms your theory
    > Oct 24 15:58:28 <alexlarsson> sberg_: alternatively you could tell your users
    >  to always build LO in the background
    > Oct 24 15:59:04 <sberg_> I'll try to put that into the release notes; lets
    >  see...
    
    Change-Id: I2a8a58f9259d2854f42f4aa3db5bb232cf70845d
    38081c08
Adı
Son kayıt (commit)
Son güncelleme
..
inc Loading commit data...
qa Loading commit data...
scripts Loading commit data...
source Loading commit data...
test/deployment Loading commit data...
uiconfig/ui Loading commit data...
unx/source Loading commit data...
util Loading commit data...
win32/source Loading commit data...
AllLangMoTarget_dkt.mk Loading commit data...
CppunitTest_desktop_app.mk Loading commit data...
CppunitTest_desktop_dialogs_test.mk Loading commit data...
CppunitTest_desktop_lib.mk Loading commit data...
CppunitTest_desktop_version.mk Loading commit data...
CustomTarget_desktop_unopackages_install.mk Loading commit data...
CustomTarget_soffice.mk Loading commit data...
Executable_minidump_upload.mk Loading commit data...
Executable_oosplash.mk Loading commit data...
Executable_quickstart.mk Loading commit data...
Executable_sbase.mk Loading commit data...
Executable_scalc.mk Loading commit data...
Executable_sdraw.mk Loading commit data...
Executable_simpress.mk Loading commit data...
Executable_smath.mk Loading commit data...
Executable_soffice.mk Loading commit data...
Executable_soffice_bin.mk Loading commit data...
Executable_sweb.mk Loading commit data...
Executable_swriter.mk Loading commit data...
Executable_unoinfo.mk Loading commit data...
Executable_unopkg.mk Loading commit data...
Executable_unopkg_bin.mk Loading commit data...
Executable_unopkg_com.mk Loading commit data...
Extension_test-active.mk Loading commit data...
Extension_test-passive.mk Loading commit data...
GeneratedPackage_desktop_unopackages_install.mk Loading commit data...
Jar_active_java.mk Loading commit data...
Jar_passive_java.mk Loading commit data...
Library_active_native.mk Loading commit data...
Library_crashreport.mk Loading commit data...
Library_deployment.mk Loading commit data...
Library_deploymentgui.mk Loading commit data...
Library_deploymentmisc.mk Loading commit data...
Library_migrationoo2.mk Loading commit data...
Library_migrationoo3.mk Loading commit data...
Library_offacc.mk Loading commit data...
Library_passive_native.mk Loading commit data...
Library_sofficeapp.mk Loading commit data...
Library_spl.mk Loading commit data...
Library_unopkgapp.mk Loading commit data...
Makefile Loading commit data...
Module_desktop.mk Loading commit data...
Package_branding.mk Loading commit data...
Package_branding_custom.mk Loading commit data...
Package_sbase_sh.mk Loading commit data...
Package_scalc_sh.mk Loading commit data...
Package_scripts.mk Loading commit data...
Package_sdraw_sh.mk Loading commit data...
Package_simpress_sh.mk Loading commit data...
Package_smath_sh.mk Loading commit data...
Package_soffice_sh.mk Loading commit data...
Package_swriter_sh.mk Loading commit data...
Pagein_calc.mk Loading commit data...
Pagein_common.mk Loading commit data...
Pagein_draw.mk Loading commit data...
Pagein_impress.mk Loading commit data...
Pagein_writer.mk Loading commit data...
Pyuno_passive_python.mk Loading commit data...
README Loading commit data...
README.vars Loading commit data...
Rdb_passive_generic.mk Loading commit data...
Rdb_passive_platform.mk Loading commit data...
StaticLibrary_winlauncher.mk Loading commit data...
StaticLibrary_winloader.mk Loading commit data...
UIConfig_deployment.mk Loading commit data...
WinResTarget_quickstart.mk Loading commit data...
WinResTarget_sbase.mk Loading commit data...
WinResTarget_scalc.mk Loading commit data...
WinResTarget_sdraw.mk Loading commit data...
WinResTarget_simpress.mk Loading commit data...
WinResTarget_smath.mk Loading commit data...
WinResTarget_soffice.mk Loading commit data...
WinResTarget_sofficebin.mk Loading commit data...
WinResTarget_sweb.mk Loading commit data...
WinResTarget_swriter.mk Loading commit data...