When an application uses libraries which use libdbus in a threaded manner, an explicit dependency on libdbus is required in order to call dbus_g_threads_init(), or things start crashing in libdbus, as seen in https://bugzilla.gnome.org/show_bug.cgi?id=683830, https://bugzilla.gnome.org/show_bug.cgi?id=659756, and https://bugs.launchpad.net/ubuntu/+source/banshee/+bug/1048341. This is rather suboptimal, as in the case of gconf, the libdbus usage is a transport that should be abstracted within the implementation details of libgconf. There should be little, if any backward compatibility breakages involved with this changed, as explained in https://bugzilla.gnome.org/show_bug.cgi?id=683830#c10.
We can't make libdbus thread-safe by default. Making it thread-safe involves memory allocations (to allocate mutexes), and libdbus claims to be robust against malloc() returning NULL, which means that "make me thread-safe" can fail. If you want thread-safety by default, use a library that doesn't claim to be robust against malloc() returning NULL, and hence can abort() if it happens, like GIO.
I checked out the dbus_thread_init_default documentation at http://dbus.freedesktop.org/doc/api/html/group__DBusThreads.html#ga33b6cf3b8f1e41bad5508f84758818a7 and it doesn't mention anything about allocations or malloc returning NULL, it just says that without it libdbus won't use locking. So what does it mean to be robust against malloc() returning NULL, and where is it documented? Does it mean that libdbus works without allocating any memory? Or does it mean that it returns an error instead of crashing the whole application? If it's the latter, then I don't think making it thread-safe by default would be any problem - the library won't work, but the application would get to decide what to do about it. If it's the former, then I think you may want to remove this requirement from the next version of libdbus - I can't imagine any application doing useful work without allocating any memory at all.
(In reply to comment #2) > So what does it mean to be robust against malloc() returning NULL, and where > is it documented? Does it mean that libdbus works without allocating any > memory? It's a requirement placed on libdbus (by its use in things like dbus-daemon, Upstart and systemd), not a requirement placed on your code that uses libdbus. However, it constrains what changes we can make to libdbus. > Or does it mean that it returns an error instead of crashing the > whole application? Yes. More specifically, it means that anything that allocates memory must be able to fail: there are lots of functions like this: /** * Do some simple thing. * * Returns: #TRUE on success, #FALSE if not enough memory. */ dbus_bool_t dbus_foo (...); whereas in a GLib-based library, you'd usually see this: /** * Do some simple thing. */ void g_foo (...); and it'd just abort() if there wasn't enough memory. If every function in libdbus that used locks behind the scenes had a boolean return value like that, then making it thread-safe by default would be no problem: each function that needs locks could call dbus_threads_init_default() before it did anything else, and if dbus_threads_init_default() failed with "not enough memory", so would the function that needed the locks. Unfortunately, there are some functions in libdbus that need to use locks for thread-safety, but do not return a value that can indicate "out of memory": notably, _dbus_atomic_inc(), dec(), get() on Unix platforms without compiler-assisted atomic operations like __sync_add_and_fetch() have this issue. There might be others. So, libdbus can't be thread-safe-by-default unless we can solve that. One possibility would be to change the fallback case for those functions to use a global pthread_mutex_t initialized with PTHREAD_MUTEX_INITIALIZER, which can be used without additional memory allocations; we already assume that Unix platforms have pthreads, so I don't think that's a portability loss. I would accept a good enough set of patches for dbus 1.7.x that changed the fallback case for Unix atomic operations to behave like that, added appropriate dbus_threads_init_default() calls to everything that needed a lock, and got rid of the current complicated infrastructure for delayed allocation of locks; ideally it would also add a dbus_global_set_single_threaded() that could be called by applications that know they are definitely single-threaded, and want to avoid the locking overhead (such as dbus-daemon itself). I don't have time to *write* that patch at the moment, though.
(In reply to comment #3) > I don't have time to *write* that patch at the moment, though. My current project ran into this bug, so I've implemented what I suggested. > (In reply to comment #2) > ideally it would also add a > dbus_global_set_single_threaded() that could be called by applications that > know they are definitely single-threaded, and want to avoid the locking > overhead (such as dbus-daemon itself) It only costs a couple of percent of throughput on my laptop, so I think life's too short to bother with this.
Created attachment 78018 [details] [review] [1] DBusAtomic: on Unix, use pthreads mutexes for fallback On pthreads platforms, POSIX guarantees that we can "allocate" mutexes as library-global variables, without involving malloc. This means we don't need to error-check their allocation - if the dynamic linker succeeds, then we have enough memory for all our globals - which is an important step towards being thread-safe by default. In particular, making atomic operations never rely on DBusMutex means that we are free to implement parts of DBusMutex in terms of DBusAtomic, if it would help. We do not currently support any non-Windows platform that does not have pthreads. This is unlikely to change. On Windows, we already used real atomic operations; we can just delete the unused global variable.
Created attachment 78019 [details] [review] [2] dbus_threads_init: call _dbus_threads_init_platform_specific() This reverses the relationship between these two functions. Previously, dbus_threads_init() wouldn't allocate dbus_cond_event_tls on Windows, call check_monotonic_clock on Unix, or call _dbus_check_setuid on Unix.
Created attachment 78020 [details] [review] dbus_threads_init_default, dbus_threads_init: be thread-safe on Unix This means that libraries that will only be used on Unix can safely call them before they call into libdbus, in the knowledge that if everyone does the same, libdbus will be thread-safe. Until someone helps to implement and test this functionality on Windows, the same cannot be said for Windows, but it's better than nothing... --- On Windows, it might be enough to have a static, never-freed CRITICAL_SECTION and use InitializeCriticalSection() from the process-attach part of DllMain() - but that only works if DllMain() is always called, even under static linkage.
Created attachment 78021 [details] [review] Statically initialize global locks to NULL This means we can rely on them not to contain junk.
Created attachment 78022 [details] [review] Add _DBUS_GNUC_WARN_UNUSED_RESULT, similar to GLib's
Created attachment 78023 [details] [review] Refactor global locks to use a simple array initialized on first use This is at least as thread-safe than what we already had: if dbus_threads_init_default() is called first, then the array is populated, and if it isn't, we populate it on first use. The remaining broken case is that two threads both start using libdbus at the same time, with no prior initialization. That's already broken, so there's nothing we can do to fix it. This makes it possible for _dbus_global_lock() (the former _DBUS_LOCK()) and _dbus_user_database_lock_system() to fail with OOM; compensate for that. This also removes the unused locks win_fds and sid_atom_cache, which were presumably once used on Windows, but are no longer mentioned anywhere. It also renames the existing static _dbus_global_lock(), _dbus_global_unlock() in dbus-sysdeps-win to _dbus_systemwide_lock(), _dbus_systemwide_unlock() which describe their semantics better (they deal with named OS-wide locks, rather than being local to a process like pthreads mutexes). --- I recognise that this is rather a mega-patch, but I'm not sure how best to split it...
Created attachment 78024 [details] [review] Always initialize threading before allocating a dynamic mutex Dynamic allocation of mutexes can fail anyway, so this is easy. Justification for not keeping the dummy mutex code-paths, even as an opt-in thing for processes known to be high-performance and single-threaded: real mutexes only cut the throughput of test/dbus-daemon.c by a couple of percent on my laptop (from around 6700 to around 6600 messages per second), and libdbus crashes caused by not calling dbus_threads_init_default() are sufficiently widespread that they're wasting a lot of everyone's time.
Created attachment 78025 [details] [review] Add a statically-initialized implementation of _dbus_global_lock() on glibc systems --- This is non-portable, but is one less thing that can fail to initialize on GNU/Linux (also GNU/kFreeBSD and GNU/Hurd, in the unlikely event that anyone cares).
(In reply to comment #12) > Created attachment 78025 [details] [review] [review] > Add a statically-initialized implementation of _dbus_global_lock() on glibc > systems I believe this makes libdbus thread-safe by default on glibc systems. If desired, we could extend this to all Unix systems at the cost of not using recursive mutexes for the global locks, by using PTHREAD_MUTEX_INITIALIZER if PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP is not available; that would extend it to "thread-safe by default on Unix, you still need to call dbus_threads_init_default() while single-threaded on Windows". If DllMain() is a viable place to allocate 12 CRITICAL_SECTIONs, we can also be thread-safe by default on Windows, by relying on DllMain() happening first. That's only reliable if it's always called in all combinations of: built with either MSVC or gcc; statically or dynamically linked to libdbus.
(In reply to comment #7) > Created attachment 78020 [details] [review] [review] > dbus_threads_init_default, dbus_threads_init: be thread-safe on Unix > > This means that libraries that will only be used on Unix can safely > call them before they call into libdbus, in the knowledge that if > everyone does the same, libdbus will be thread-safe. > > Until someone helps to implement and test this functionality on Windows, > the same cannot be said for Windows, but it's better than nothing... > > --- > > On Windows, it might be enough to have a static, never-freed > CRITICAL_SECTION and use InitializeCriticalSection() from the process-attach > part of DllMain() - but that only works if DllMain() is always called, even > under static linkage. DllMain() is only usable when building dynamic libdbus library. Alternatives I see are - using a static global c++ object, which is called from the c runtime before main class DBusInitialisation { public: DBusInitialisation() { /* run initialisation code */ } }; static DBusInitialisation globalObject; This works with msvc and mingw and there are no additional libraries linked into like libstdc++ or similar. - an init-on-the-first-dbus-function-call approach. for example in ... dbus_bus_get(...) { if (!dbus_is_initiated) /* run init */ ... normal operation }
(In reply to comment #14) > - using a static global c++ object, which is called from the c runtime > before main That's analogous to __attribute__(__constructor__) in gcc, right? That might be the best way to do this setup, if it's guaranteed to be called either before main() (when statically linked) or under the same lock as DllMain() (when dynamically linked). > - an init-on-the-first-dbus-function-call approach. ... > ... dbus_bus_get(...) > { > if (!dbus_is_initiated) > /* run init */ That's not thread-safe. Think about what happens if threads 2 and 3 are the first users of D-Bus, and both enter dbus_bus_get() at about the first time: it is possible that they both see dbus_is_initiated == FALSE, both enter the initialization block, and allocate and lock different mutexes while thinking they have the same mutex. My patches on this bug already do the equivalent of this: they attempt to initialize locking on the first call to _dbus_global_lock() (the former _DBUS_LOCK()) or the first allocation of a dynamically-allocated mutex or condition variable. It still isn't thread-safe, but you're right that it seems less bad than our current situation.
(In reply to comment #15) > (In reply to comment #14) > > - using a static global c++ object, which is called from the c runtime > > before main > > That's analogous to __attribute__(__constructor__) in gcc, right? According to <http://stackoverflow.com/questions/4496233/which-is-called-first-dllmain-or-global-static-object-constructor> this seems like a good possibility, if you don't mind saying "on Windows, libdbus now requires a C++ compiler". Since our only supported compilers on Windows are gcc (which comes with g++) and MSVC (which is a better C++ compiler than it is a C compiler), that seems reasonable.
(In reply to comment #15) > (In reply to comment #14) > > - using a static global c++ object, which is called from the c runtime > > before main > > That's analogous to __attribute__(__constructor__) in gcc, right? did not uses that before, but after reading from http://stackoverflow.com/questions/2053029/how-exactly-does-attribute-constructor-work I would say yes. Unfortunally this is compiler specific.
(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #14) > > > - using a static global c++ object, which is called from the c runtime > > > before main > > > > That's analogous to __attribute__(__constructor__) in gcc, right? > > According to > <http://stackoverflow.com/questions/4496233/which-is-called-first-dllmain-or- > global-static-object-constructor> this seems like a good possibility, if you > don't mind saying "on Windows, libdbus now requires a C++ compiler". > > Since our only supported compilers on Windows are gcc (which comes with g++) The mingw-w32 binary package from http://sourceforge.net/projects/mingw-w64/ contains a c++ compiler, mingw4 and cygwin provides g++ in a separate binary package, which needs to be listed as requirement. > and MSVC (which is a better C++ compiler than it is a C compiler), > that seems reasonable. yes.
(In reply to comment #15) > (In reply to comment #14) > > - using a static global c++ object, which is called from the c runtime > > before main > > That's analogous to __attribute__(__constructor__) in gcc, right? > > That might be the best way to do this setup, if it's guaranteed to be called > either before main() (when statically linked) or under the same lock as > DllMain() (when dynamically linked). > > > - an init-on-the-first-dbus-function-call approach. > ... > > ... dbus_bus_get(...) > > { > > if (!dbus_is_initiated) > > /* run init */ > > That's not thread-safe. Think about what happens if threads 2 and 3 are the > first users of D-Bus, and both enter dbus_bus_get() at about the first time: > it is possible that they both see dbus_is_initiated == FALSE, both enter the > initialization block, and allocate and lock different mutexes while thinking > they have the same mutex. > > My patches on this bug already do the equivalent of this: they attempt to > initialize locking on the first call to _dbus_global_lock() (the former > _DBUS_LOCK()) or the first allocation of a dynamically-allocated mutex or > condition variable. It still isn't thread-safe, but you're right that it > seems less bad than our current situation. So the above mentioned example needs a locking wrapper around > > if (!dbus_is_initiated) > > /* run init */
(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #14) > > > - using a static global c++ object, which is called from the c runtime > > > before main > > > > That's analogous to __attribute__(__constructor__) in gcc, right? > > According to > <http://stackoverflow.com/questions/4496233/which-is-called-first-dllmain-or- > global-static-object-constructor> this seems like a good possibility, if you > don't mind saying "on Windows, libdbus now requires a C++ compiler". no see comment 18
(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #14) > > > - using a static global c++ object, which is called from the c runtime > > > before main > > > > That's analogous to __attribute__(__constructor__) in gcc, right? > > According to > <http://stackoverflow.com/questions/4496233/which-is-called-first-dllmain-or- > global-static-object-constructor> this seems like a good possibility, if you > don't mind saying "on Windows, libdbus now requires a C++ compiler". > > Since our only supported compilers on Windows are gcc (which comes with g++) > and MSVC (which is a better C++ compiler than it is a C compiler), that > seems reasonable. so I guess the static global object has to be added to dbus-sysdeps-thread-win.c and need to call dbus_threads_init_default() right ? When adding such an object I guess the remaining tls related code in dllMain() need to be refactored too ? /* We need this to free the TLS events on thread exit */ BOOL WINAPI DllMain (hinst_t hinstDLL, DWORD fdwReason, LPVOID lpvReserved) { HANDLE event; switch (fdwReason) { case DLL_PROCESS_ATTACH: dbus_dll_hmodule = hinstDLL; break; case DLL_THREAD_DETACH: if (dbus_cond_event_tls != TLS_OUT_OF_INDEXES) { event = TlsGetValue(dbus_cond_event_tls); CloseHandle (event); TlsSetValue(dbus_cond_event_tls, NULL); } break; case DLL_PROCESS_DETACH: if (dbus_cond_event_tls != TLS_OUT_OF_INDEXES) { event = TlsGetValue(dbus_cond_event_tls); CloseHandle (event); TlsSetValue(dbus_cond_event_tls, NULL); TlsFree(dbus_cond_event_tls); } The related objects are initiated in _dbus_threads_init_platform_specific()=, which is called from dbus_thread_init(), which can be destroyed in the destructor of the global object. Still not sure, if the freeing on thread detach is required. At least in static builds this code will never called because dllMain() may not be included.
(In reply to comment #19) > So the above mentioned example needs a locking wrapper around > > > if (!dbus_is_initiated) > > > /* run init */ Right, but you can't have a locking wrapper without locks, and if you can't have locks without initializing them... you see the problem? :-) (In reply to comment #21) > so I guess the static global object has to be added to > dbus-sysdeps-thread-win.c and need to call dbus_threads_init_default() right > ? I'll attach a patch for review/testing in a moment (I'm redoing the patches with your suggestion in mind), but my answer is: no it shouldn't call dbus_threads_init_default(), because that can fail if there isn't enough memory, and in a global constructor there's no way to report the error. However, as far as I can see, InitializeCriticalSection can't fail, so it's OK to use in a context where errors can't be reported. The requirement that libdbus survives out-of-memory and similar conditions is only really relevant to Unix system services, so if you want to introduce a policy of "under Windows, certain cases of out-of-memory and similar conditions are treated as fatal and will just abort()" I wouldn't object. > When adding such an object I guess the remaining tls related code in > dllMain() need to be refactored too ? My intention was to leave it how it is for now - if it works correctly, we don't need to change it - but I'd be willing to review tested patches. Can we land my changes first, though? > The related objects are initiated in > _dbus_threads_init_platform_specific()=, which is called from > dbus_thread_init(), which can be destroyed in the destructor of the global > object. My inclination is to have the global object just not have a destructor. If it will only be freed when the process exits, then ... all of those resources are freed anyway, so it's useless. Beyond a certain point, freeing globals just adds complexity. (We do semi-support dbus_shutdown() for the benefit of valgrind users, but it should never be called automatically.)
Created attachment 78086 [details] [review] dbus_threads_init_default, dbus_threads_init: be safe to call at any time On Unix, we use a pthreads mutex, which can be allocated and initialized in global memory. On Windows, we use a CRITICAL_SECTION, together with a call to InitializeCriticalSection() from the constructor of a global static C++ object (thanks to Ralf Habacker for suggesting this approach). --- Windows version compiled with mingw and doesn't obviously fail horribly, but not properly tested: the tests don't fully work under Wine yet.
Created attachment 78087 [details] [review] [3 v3] dbus_threads_init_default, dbus_threads_init: be safe to call at any time On Unix, we use a pthreads mutex, which can be allocated and initialized in global memory. On Windows, we use a CRITICAL_SECTION, together with a call to InitializeCriticalSection() from the constructor of a global static C++ object (thanks to Ralf Habacker for suggesting this approach). --- Sorry, that was the wrong version of that patch; here's a better one.
Created attachment 78088 [details] [review] [4] Remove unused global mutexes for win_fds, sid_atom_cache --- I assume these were for Windows, once? Nothing mentions them any more.
Created attachment 78089 [details] [review] [5] Turn a runtime assertion into a compile-time assertion
Created attachment 78091 [details] [review] [6] Replace individual global-lock variables with an array of DBusRMutex * This means we can use a much simpler code structure in data-slot allocators: instead of giving them a DBusRMutex ** at first-allocation, we can just give them an index into the array, which can be done statically. It doesn't make us any more thread-safe-by-default - the mutexes will only actually be used if threads were already initialized - but it's substantially better than nothing. These locks really do have to be recursive: for instance, internal_bus_get() calls dbus_bus_register() under the bus lock, and dbus_bus_register() can call _dbus_connection_close_possibly_shared(), which takes the bus lock.
Created attachment 78093 [details] [review] [7] Add _DBUS_GNUC_WARN_UNUSED_RESULT, similar to GLib's
Created attachment 78094 [details] [review] [8] Make taking a global lock automatically initialize locking if needed This lets them be thread-safe by default, at the cost that they can now fail. --- init_global_locks(), init_uninitialized_locks() now have to use a new _dbus_register_shutdown_func_unlocked() and do the locking themselves, so that they don't deadlock. Before documenting libdbus as thread-safe by default, I still need to prove to my satisfaction that this: + if (thread_init_generation != _dbus_current_generation && + !dbus_threads_init_default ()) + return FALSE; + + _dbus_platform_rmutex_lock (global_locks[lock]); + return TRUE; is thread-safe. If it unconditionally called dbus_threads_init_default(), it'd be taking and releasing a global mutex every time it wanted to take a different global mutex, rather defeating the point of not having a "big kernel lock"-equivalent - although the mutex used by dbus_threads_init_default() on Unix is unsuitable for at least _DBUS_LOCK(bus), because it's not recursive. I believe this is thread-safe usage - thread_init_generation is only updated while under a lock, so we have a memory barrier whenever it changes - at least when dbus_shutdown() is not used incorrectly. You basically can't ever call dbus_shutdown() in a threaded application that loads arbitrary code, because you don't know that your other threads aren't using libdbus.
Created attachment 78095 [details] [review] [9] Always initialize threading before allocating a dynamic mutex Dynamic allocation of mutexes can fail anyway, so this is easy. Justification for not keeping the dummy mutex code-paths, even as an opt-in thing for processes known to be high-performance and single-threaded: real mutexes only cut the throughput of test/dbus-daemon.c by a couple of percent on my laptop (from around 6700 to around 6600 messages per second), and libdbus crashes caused by not calling dbus_threads_init_default() are sufficiently widespread that they're wasting a lot of everyone's time.
Created attachment 78096 [details] [review] [10] Add a statically-initialized implementation of _dbus_lock() on glibc systems
Created attachment 78097 [details] [review] [11] [untested] add an automatically-initialized implementation of _dbus_lock on Windows --- I don't propose to merge this unless Ralf reviews and tests it and is happy that it's OK, because it isn't on the critical path to get libdbus thread-safe by default - but it would be nice to have.
(In reply to comment #18) > > > > Since our only supported compilers on Windows are gcc (which comes with g++) > > The mingw-w32 binary package from http://sourceforge.net/projects/mingw-w64/ > contains a c++ compiler, mingw4 and cygwin provides g++ in a separate binary > package, which needs to be listed as requirement. Not really correct - cygwin is unix, so this requirement do not apply
Created attachment 78137 [details] [review] cmake build fix
(In reply to comment #32) > Created attachment 78097 [details] [review] [review] > [11] [untested] add an automatically-initialized implementation of > _dbus_lock on Windows > > --- > > I don't propose to merge this unless Ralf reviews and tests it and is happy > that it's OK, because it isn't on the critical path to get libdbus > thread-safe by default - but it would be nice to have. added a dbus_verbose message in the constructor and compiled msvc: dbus-daemon: [dbus\dbus-init-win.cpp(38):DBusInternalInit::DBusInternalInit] global constructor called ... dbus-monitor: [dbus\dbus-init-win.cpp(38):DBusInternalInit::DBusInternalInit] global constructor called .. dbus-daemon and dbus-monitor reported both the calling of the constructor, looks good. BTW: dbus-daemon rejected to start first because of the lack of dbus_message_unix_fds limit value in generated session.conf (see additional patch)
(In reply to comment #35) > dbus-daemon and dbus-monitor reported both the calling of the constructor, > looks good. Great. (In reply to comment #33) > > The mingw-w32 binary package from http://sourceforge.net/projects/mingw-w64/ > > contains a c++ compiler, mingw4 and cygwin provides g++ in a separate binary > > package, which needs to be listed as requirement. > > Not really correct - cygwin is unix, so this requirement do not apply Right, the Cygwin runtime implements the standard pthreads API in terms of Windows thread primitives, I think. Here is a proposed NEWS: +Dependencies: + +• The Windows version of libdbus now contains a C++ source file, required + for correct global initialization. gcc (mingw*) users should ensure + that g++ is installed. Does that look OK to you?
Comment on attachment 78137 [details] [review] cmake build fix Review of attachment 78137 [details] [review]: ----------------------------------------------------------------- > Also converted related variable to upper case as already used in other places. If you insist on upper-case, please call it DEFAULT_MESSAGE_UNIX_FDS (not MESSAGES in the middle). Using singular MESSAGE is more accurate - it's the (default for the) maximum number of Unix fds (plural) per message (singular) - and the MESSAGE form reads better in English anyway. The things for which it's a default are all of the form (something)_message_unix_fds, too. ::: cmake/CMakeLists.txt @@ +434,4 @@ > #AC_ARG_WITH(dbus_user, AS_HELP_STRING([--with-dbus-user=<user>],[User for running the DBUS daemon (messagebus)])) > > set (DBUS_USER ) > +set (DEFAULT_MESSAGES_UNIX_FDS 1024) I see the problem: the patch that added this added it to config.h.cmake, not to CMakeLists.txt, whereas substitution into the .in file requires this form. Sorry about that. Should it be turned into a #cmakedefine in the .h.cmake file? ::: configure.ac @@ +1305,2 @@ > AC_DEFINE_UNQUOTED([DBUS_DEFAULT_MESSAGE_UNIX_FDS], > [$default_message_unix_fds], If you're going to rename default_message_unix_fds to something else, the line with [$default_message_unix_fds] also needs the same renaming to be applied to it.
(In reply to comment #37) > Comment on attachment 78137 [details] [review] [review] > cmake build fix > > Review of attachment 78137 [details] [review] [review]: > ----------------------------------------------------------------- > > > Also converted related variable to upper case as already used in other places. > > If you insist on upper-case, This is not my definition, all previously used replacement macros in *.in files are using upper case, so why should a lower case 'default_message_unix_fds' be used ? > please call it DEFAULT_MESSAGE_UNIX_FDS (not MESSAGES in the middle). sure, oversaw this. > ::: cmake/CMakeLists.txt > @@ +434,4 @@ > > #AC_ARG_WITH(dbus_user, AS_HELP_STRING([--with-dbus-user=<user>],[User for running the DBUS daemon (messagebus)])) > > > > set (DBUS_USER ) > > +set (DEFAULT_MESSAGES_UNIX_FDS 1024) > > I see the problem: the patch that added this added it to config.h.cmake, not > to CMakeLists.txt, whereas substitution into the .in file requires this > form. Sorry about that. Should it be turned into a #cmakedefine in the > .h.cmake file? yes > > ::: configure.ac > @@ +1305,2 @@ > > AC_DEFINE_UNQUOTED([DBUS_DEFAULT_MESSAGE_UNIX_FDS], > > [$default_message_unix_fds], > > If you're going to rename default_message_unix_fds to something else, the > line with [$default_message_unix_fds] also needs the same renaming to be > applied to it. sure, oversaw this
(In reply to comment #37) > cmake build fix Let's do this separately, since it isn't particularly thread-safety-related. Bug #63682. > > If you insist on upper-case, > This is not my definition, all previously used replacement macros in *.in > files are using upper case, so why should a lower case > 'default_message_unix_fds' be used ? Yeah, you're right. See the other bug for patches.
I read the first 5 patches (except the dbus-init-win.cpp) part and I am not a maintainer but it looks good to me.
Comment on attachment 78018 [details] [review] [1] DBusAtomic: on Unix, use pthreads mutexes for fallback Review of attachment 78018 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps-unix.c @@ +2432,5 @@ > } > > #if !DBUS_USE_SYNC > +/* To be thread-safe by default on platforms that don't necessarily have > + * atomic operations (notably Debian armel, which is armv4t), we must I'll test this on a Debian armel machine before merging - armv4t is probably about the only platform where this code path is taken.
Comment on attachment 78091 [details] [review] [6] Replace individual global-lock variables with an array of DBusRMutex * Review of attachment 78091 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-internals.h @@ +295,5 @@ > typedef void (* DBusShutdownFunction) (void *data); > +dbus_bool_t _dbus_register_shutdown_func (DBusShutdownFunction function, > + void *data); > +dbus_bool_t _dbus_register_shutdown_func_unlocked (DBusShutdownFunction function, > + void *data); Strictly speaking, _dbus_register_shutdown_func_unlocked() doesn't need to be added until the next commit, I don't think... but this is the version I've tested, and in any case, it doesn't hurt. ::: dbus/dbus-threads.c @@ +496,4 @@ > > + _dbus_lock (_DBUS_LOCK_NAME (shutdown_funcs)); > + ok = _dbus_register_shutdown_func_unlocked (shutdown_global_locks, NULL); > + _dbus_unlock (_DBUS_LOCK_NAME (shutdown_funcs)); This could in fact still be a call to _dbus_register_shutdown_func(), or could use _DBUS_LOCK() and _DBUS_UNLOCK(). However, the difference is only cosmetic, and the next commit needs to use _dbus_register_shutdown_func_unlocked() and inline the locking code anyway, to avoid infinite recursion... so I don't think it's worth editing this commit.
(In reply to comment #42) > Strictly speaking, _dbus_register_shutdown_func_unlocked() doesn't need to > be added until the next commit ... > the next commit needs to use [...] where by the "next" I meant "the one after the next", but you get the idea.
Comment on attachment 78094 [details] [review] [8] Make taking a global lock automatically initialize locking if needed Review of attachment 78094 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-threads.c @@ +426,5 @@ > > + /* This assumes that init_global_locks() has already been called. */ > + _dbus_platform_rmutex_lock (global_locks[_DBUS_LOCK_shutdown_funcs]); > + ok = _dbus_register_shutdown_func_unlocked (shutdown_uninitialized_locks, NULL); > + _dbus_platform_rmutex_unlock (global_locks[_DBUS_LOCK_shutdown_funcs]); This maybe deserves a bit of explanation in the long commit message, something like this (I'll 'reword' while committing). init_uninitialized_locks() and init_global_locks() must now both reimplement the equivalent of _dbus_register_shutdown_func(), by using _dbus_platform_rmutex_lock() on the same underlying mutex around a call to _dbus_register_shutdown_func_unlocked(). This is because if they used the usual _DBUS_LOCK() API (as _dbus_register_shutdown_func() does), it would automatically try to initialize global locking, leading to infinite recursion. @@ +504,2 @@ > ok = _dbus_register_shutdown_func_unlocked (shutdown_global_locks, NULL); > + _dbus_platform_rmutex_unlock (global_locks[_DBUS_LOCK_shutdown_funcs]); Same comments as above, except that as noted in patch 6, this call-site was already (trivially) reimplementing _dbus_register_shutdown_func(). @@ +585,4 @@ > } > > if (!_dbus_threads_init_platform_specific() || > + /* init_global_locks() must be called before init_uninitialized_locks. */ This is because init_global_locks() populates global_locks[], and init_uninitialized_locks() uses it. ::: dbus/dbus-userdb-util.c @@ +162,5 @@ > { > DBusUserDatabase *db; > const DBusGroupInfo *info; > + > + /* FIXME: this can't distinguish ENOMEM from other errors */ Most of the dbus-userdb functions have this design flaw, but it isn't a regression - they already don't distinguish ENOMEM from other error cases anyway - so I'm not going to try to fix that right now.
Comment on attachment 78095 [details] [review] [9] Always initialize threading before allocating a dynamic mutex Review of attachment 78095 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-threads.c @@ +189,5 @@ > > /** > * This does the same thing as _dbus_condvar_new. It however > * gives another level of indirection by allocating a pointer > + * to point to the condvar location; this used to be useful. Maybe one day I'll adjust the callers to use the obvious calling convention again. Today is not that day.
Comment on attachment 78096 [details] [review] [10] Add a statically-initialized implementation of _dbus_lock() on glibc systems Review of attachment 78096 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-threads-internal.h @@ +35,5 @@ > +/* glibc can implement global locks without needing an initialization step, > + * which improves our thread-safety-by-default further. */ > +#if defined(__GLIBC__) && defined(PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP) > +# define DBUS_HAVE_STATIC_RECURSIVE_MUTEXES 1 > +#endif If other platforms have this feature (hello Android, *BSD and Solaris fans), I would welcome patches to add them to this special case; but I'm not going to do that myself for platforms I can't test. If PTHREAD_RECURSIVE_MUTEX_INITIALIZER is ever standardized, or if other platforms have it anyway, I'd be open to requests to use that.
I read the patches up to 10 and I am not a maintainer but I only have nitpicks: - the commit logs don't include the bug URL - the test suite test on values like "0xABCDEF" which are not meaningful anymore. It is harmless but looks weird; it could be fixed in a later patch.
Comment on attachment 78091 [details] [review] [6] Replace individual global-lock variables with an array of DBusRMutex * Review of attachment 78091 [details] [review]: ----------------------------------------------------------------- Patch seems reasonable, although I have no idea how to test this. ::: cmake/Doxyfile.cmake @@ -147,4 @@ > "DBUS_END_DECLS=" \ > "DOXYGEN_SHOULD_SKIP_THIS" \ > "DBUS_GNUC_DEPRECATED=" \ > - "_DBUS_DEFINE_GLOBAL_LOCK(name)=" \ probably off-topic: cmake could also use DoxyFile.in
Comment on attachment 78093 [details] [review] [7] Add _DBUS_GNUC_WARN_UNUSED_RESULT, similar to GLib's Review of attachment 78093 [details] [review]: ----------------------------------------------------------------- looks good
(In reply to comment #47) > - the commit logs don't include the bug URL Yeah, I'll add that (along with an appropriate Reviewed-by) when I merge them. > - the test suite test on values like "0xABCDEF" which are not meaningful > anymore. It is harmless but looks weird; it could be fixed in a later patch. I'll append a patch for that.
(In reply to comment #41) > Comment on attachment 78018 [details] [review] > [1] DBusAtomic: on Unix, use pthreads mutexes for fallback > > > #if !DBUS_USE_SYNC > > +/* To be thread-safe by default on platforms that don't necessarily have > > + * atomic operations (notably Debian armel, which is armv4t), we must > > I'll test this on a Debian armel machine before merging - armv4t is probably > about the only platform where this code path is taken. This can be tested without ARM hardware by setting dbus_cv_sync_sub_and_fetch=no on the configure command line, in fact. From the department of "if it isn't tested, it doesn't work", it turns out that when you do that, the regression tests fail without this patch (because we try to do an atomic operation after dbus_shutdown() frees the necessary mutex, or something), and pass with it. So this patch is certainly an improvement, and I would appreciate review from another maintainer, even if you don't have time to review the whole series. This patch is already Reviewed-by: Alban (but he isn't a D-Bus maintainer). It should also get the bug number in the commit message, but I'll add that when I commit.
Comment on attachment 78093 [details] [review] [7] Add _DBUS_GNUC_WARN_UNUSED_RESULT, similar to GLib's Applied for 1.7.4, thanks. Any reviews for the rest of the patch series?
(In reply to comment #48) > probably off-topic: cmake could also use DoxyFile.in That would be great, but I'm not going to block this bugfix on it. Patches welcome, please spin it off to a separate bug.
Rebased patches can be seen at <http://cgit.freedesktop.org/~smcv/dbus/log/?h=thread-safe4>. I won't merge the one marked "[untested]" unless someone confirms that it works; the rest are ready for merge once a maintainer has reviewed them. There's one new patch for this bug (which I'll attach in a moment), and the patch for Bug #64362 at the beginning of the branch. Other than that, no code changes, just extra metadata in the commit messages.
Created attachment 79028 [details] [review] [12] Stop asserting that we're not using the dummy lock implementation That implementation no longer exists, so neither 0xABCDEF nor 0xABCDEF2 has any special meaning any more. --- Alban suggested this. I agree it makes sense.
Comment on attachment 78018 [details] [review] [1] DBusAtomic: on Unix, use pthreads mutexes for fallback Review of attachment 78018 [details] [review]: ----------------------------------------------------------------- looks good.
Comment on attachment 78019 [details] [review] [2] dbus_threads_init: call _dbus_threads_init_platform_specific() Review of attachment 78019 [details] [review]: ----------------------------------------------------------------- I understand, that this patch changes the calling order from dbus_thread_init_default()|debug() calls _dbus_threads_init_platform_specific(), which calls dbus_thread_init() to dbus_thread_init_default()|debug() calls _dbus_threads_init() which calls dbus_thread_init_platform_specific() which looks good.
Comment on attachment 78018 [details] [review] [1] DBusAtomic: on Unix, use pthreads mutexes for fallback (In reply to comment #56) > [1] DBusAtomic: on Unix, use pthreads mutexes for fallback > looks good. Thanks, pushed.
Comment on attachment 78019 [details] [review] [2] dbus_threads_init: call _dbus_threads_init_platform_specific() (In reply to comment #57) > [2] dbus_threads_init: call _dbus_threads_init_platform_specific() > > I understand, that this patch changes the calling order > from dbus_thread_init_default()|debug() calls > _dbus_threads_init_platform_specific(), which calls dbus_thread_init() > to dbus_thread_init_default()|debug() calls _dbus_threads_init() which > calls dbus_thread_init_platform_specific() > which looks good. Yes, that's the idea. Thanks, pushed.
Comment on attachment 78087 [details] [review] [3 v3] dbus_threads_init_default, dbus_threads_init: be safe to call at any time Review of attachment 78087 [details] [review]: ----------------------------------------------------------------- There is code where unlocking connection will fail when dbus is shutdown _dbus_connection_ref_unlocked (DBusConnection *connection) { dbus_int32_t old_refcount; _dbus_assert (connection != NULL); _dbus_assert (connection->generation == _dbus_current_generation); ... or connection are not closed in that case _dbus_connection_close_possibly_shared (DBusConnection *connection) { _dbus_assert (connection != NULL); _dbus_assert (connection->generation == _dbus_current_generation); CONNECTION_LOCK (connection); _dbus_connection_close_possibly_shared_and_unlock (connection); } will this not generate memory leaks ?
thread-safe4 branch also rebased onto what's been merged, and updated with a longer (and hopefully clearer) commit message for patch 8, using some of the text of Comment #44.
(In reply to comment #60) > There is code where unlocking connection will fail when dbus is shutdown > > _dbus_connection_ref_unlocked (DBusConnection *connection) > { > dbus_int32_t old_refcount; > > _dbus_assert (connection != NULL); > _dbus_assert (connection->generation == _dbus_current_generation); > ... > > or connection are not closed in that case Please explain further? I don't see the problem here, but maybe I'm missing something important. It is considered a programming error to call dbus_shutdown() if any module in your application is still using libdbus for anything, including "still has a pointer to a non-freed DBusConnection". In practice, that means that only single-threaded applications can call dbus_shutdown(), and then only at the end of main(), and even then it can be wrong in pathological situations. We are meant to support stopping use of libdbus, calling dbus_shutdown(), then starting using libdbus again (in a new "generation") - I don't think anything outside D-Bus' own regression tests actually does this. I do wonder whether dbus_shutdown() is more trouble than it's worth and we should just have Valgrind suppressions for the process-global allocations, a dbus_shutdown() that does a lot less, and an internal-only _dbus_shutdown() for the regression tests. "Shut down the world" functions are basically harmful (compare <https://lists.fedoraproject.org/pipermail/devel/2010-January/129117.html>). > _dbus_connection_close_possibly_shared (DBusConnection *connection) > { > _dbus_assert (connection != NULL); > _dbus_assert (connection->generation == _dbus_current_generation); > > CONNECTION_LOCK (connection); > _dbus_connection_close_possibly_shared_and_unlock (connection); > } > > will this not generate memory leaks ? What is leaked, under which circumstances?
(In reply to comment #62) > (In reply to comment #60) > > There is code where unlocking connection will fail when dbus is shutdown > > > > _dbus_connection_ref_unlocked (DBusConnection *connection) > > { > > dbus_int32_t old_refcount; > > > > _dbus_assert (connection != NULL); > > _dbus_assert (connection->generation == _dbus_current_generation); > > ... > > > > or connection are not closed in that case > > Please explain further? I don't see the problem here, but maybe I'm missing > something important. > > It is considered a programming error to call dbus_shutdown() if any module > in your application is still using libdbus for anything, including "still > has a pointer to a non-freed DBusConnection". > > In practice, that means that only single-threaded applications can call > dbus_shutdown(), and then only at the end of main(), and even then it can be > wrong in pathological situations. > > We are meant to support stopping use of libdbus, calling dbus_shutdown(), > then starting using libdbus again (in a new "generation") - I don't think > anything outside D-Bus' own regression tests actually does this. > > I do wonder whether dbus_shutdown() is more trouble than it's worth and we > should just have Valgrind suppressions for the process-global allocations, a > dbus_shutdown() that does a lot less, and an internal-only _dbus_shutdown() > for the regression tests. "Shut down the world" functions are basically > harmful (compare > <https://lists.fedoraproject.org/pipermail/devel/2010-January/129117.html>). > > > _dbus_connection_close_possibly_shared (DBusConnection *connection) > > { > > _dbus_assert (connection != NULL); > > _dbus_assert (connection->generation == _dbus_current_generation); > > > > CONNECTION_LOCK (connection); > > _dbus_connection_close_possibly_shared_and_unlock (connection); > > } > > > > will this not generate memory leaks ? > > What is leaked, under which circumstances? This relates still to connections, although I pasted the wrong code snippets. The issue I see is in void dbus_connection_unref (DBusConnection *connection) { dbus_int32_t old_refcount; _dbus_return_if_fail (connection != NULL); _dbus_return_if_fail (connection->generation == _dbus_current_generation); old_refcount = _dbus_atomic_dec (&connection->refcount); where the connection may be not unreferenced and final not been free'd because of _dbus_return_if_fail (connection->generation == _dbus_current_generation); The same thing occurs in dbus_connection_close(), dbus_message_unref()
Comment on attachment 78089 [details] [review] [5] Turn a runtime assertion into a compile-time assertion Review of attachment 78089 [details] [review]: ----------------------------------------------------------------- If _DBUS_STATIC_ASSERT do what I see from the source (it raises a compile time error by adding an invalid variable definition), this looks good.
(In reply to comment #63) > The issue I see is in [...] dbus_connection_unref [...] > where the connection may be not unreferenced and final not been free'd > because of > > _dbus_return_if_fail (connection->generation == _dbus_current_generation); If a _dbus_return_if_fail() fails, the intention is that's a bug in code calling into libdbus - for instance, a function is documented to take a UTF-8 string parameter, but you gave it NULL or non-UTF-8 - and the resolution is to fix the application (or higher-level library). In this case, the incorrect use of libdbus is "keeping a DBusConnection from one generation to the next". The only thing that changes _dbus_current_generation (as far as I can see) is dbus_shutdown() in dbus-memory.c, and this sequence (which I think is the one you're worried about): DBusConnection *my_conn = dbus_connection_open (...); dbus_shutdown (my_conn); dbus_connection_unref (my_conn); (or the same with a DBusMessage) is explicitly wrong, because the documentation says: * You MUST free all memory and release all reference counts * returned to you by libdbus prior to calling dbus_shutdown(). ... * You can't continue to use any D-Bus objects, such as connections, * that were allocated prior to dbus_shutdown(). ... * WARNING: dbus_shutdown() is NOT thread safe, it must be called * while NO other threads are using D-Bus. ... * You have to know that nobody is using libdbus in your application's * process before you can call dbus_shutdown(). One implication of this * is that calling dbus_shutdown() from a library is almost certainly * wrong, since you don't know what the rest of the app is up to. In general, calling dbus_shutdown() is a bad idea: I don't think it's reasonable to call it anywhere other than "in a regression test, just before you leave main()". Does that address your concerns, or is there some other problem here that I haven't spotted?
(In reply to comment #65) > If a _dbus_return_if_fail() fails, the intention is that's a bug in code > calling into libdbus It's the same design as g_return_[val_]if_fail(), for which see <https://bugzilla.gnome.org/show_bug.cgi?id=660809>
(In reply to comment #65) > (In reply to comment #63) > > The issue I see is in [...] dbus_connection_unref [...] > > where the connection may be not unreferenced and final not been free'd > > because of > > > > _dbus_return_if_fail (connection->generation == _dbus_current_generation); > > If a _dbus_return_if_fail() fails, the intention is that's a bug in code > calling into libdbus - for instance, a function is documented to take a > UTF-8 string parameter, but you gave it NULL or non-UTF-8 - and the > resolution is to fix the application (or higher-level library). > > In this case, the incorrect use of libdbus is "keeping a DBusConnection from > one generation to the next". The only thing that changes > _dbus_current_generation (as far as I can see) is dbus_shutdown() in > dbus-memory.c, and this sequence (which I think is the one you're worried > about): > yes > In general, calling dbus_shutdown() is a bad idea: I don't think it's > reasonable to call it anywhere other than "in a regression test, just before > you leave main()". At least on windows applications may load the dbus-1.dll with LoadLibrary and unload it later with FreeLibrary. I guess this will such a use case. Before unloading dbus_shutdown() should be called ? > Does that address your concerns, yes > or is there some other problem here that I haven't spotted? no so patch looks good.
(In reply to comment #67) > At least on windows applications may load the dbus-1.dll with LoadLibrary > and unload it later with FreeLibrary. I guess this will such a use case. > Before unloading dbus_shutdown() should be called ? You can do the same with dlopen() on Unix, but using FreeLibrary() or dlclose() is already pretty problematic, particularly in the presence of unknown modules (and doubly so if you have threads). I would guess that "most" libraries don't support being used like that; GLib certainly doesn't. If you do, libdbus will leak global memory unless you dbus_shutdown(), but I don't think that's a reason to use dbus_shutdown(); rather, I think it's a reason not to unload it. FreeLibrary() and dlopen() use reference-counting to avoid problems when two modules both use a particular library, but dbus_shutdown() doesn't (and can't, really - there's no single entry point). The reliable and portable thing to do is to divide libraries into two categories: "real libraries" (like libdbus, GLib, libc, libQtCore) which you link with -ldbus-1 or whatever, and plugins which you dlopen() or LoadLibrary(). Loading "real libraries" with dlopen() is not portable (although it works fine on at least Linux, and the Windows equivalent with LoadLibrary() is also OK). When using GNU ld, it's possible to mark a library as impossible to unload, so dlclose() does nothing (ld -z nodelete, or more commonly $(CC) -Wl,-z,nodelete). I previously argued that plugins that are linked to libdbus should mark *themselves* as "nodelete", since the plugin is the thing using unusual library loading (which has the effect of preventing libdbus from being unloaded too, since the plugin holds a reference), but perhaps I was wrong and libdbus should be built with nodelete after all. Does MSVC have an equivalent of linking with the nodelete option?
Comment on attachment 78087 [details] [review] [3 v3] dbus_threads_init_default, dbus_threads_init: be safe to call at any time (In reply to comment #67) > so patch looks good. Fixed in git for 1.7.4, thanks. This is enough to be a useful increase in thread-safety - plugins can now call dbus_threads_init_default() in their initialization - so I'm tempted to do a 1.7.4 release fairly soon. The rest of the patch series would still be useful, to avoid the "pointless library init function" anti-pattern.
(In reply to comment #68) > (In reply to comment #67) > > At least on windows applications may load the dbus-1.dll with LoadLibrary > > and unload it later with FreeLibrary. I guess this will such a use case. > > Before unloading dbus_shutdown() should be called ? > > You can do the same with dlopen() on Unix, but using FreeLibrary() or > dlclose() is already pretty problematic, particularly in the presence of > unknown modules (and doubly so if you have threads). I would guess that > "most" libraries don't support being used like that; GLib certainly doesn't. On windows there is a concept to deal with this running-threads-before-dynamically-load-dll issues - http://msdn.microsoft.com/en-us/library/windows/desktop/ms686997%28v=vs.85%29.aspx which is linked from http://msdn.microsoft.com/en-us/library/windows/desktop/ms685090%28v=vs.85%29.aspx > If you do, libdbus will leak global memory unless you dbus_shutdown(), this is what I assumed > but I don't think that's a reason to use dbus_shutdown(); rather, I think it's a > reason not to unload it. Should a dynamical loaded library not free all allocated memory on unload ? > FreeLibrary() and dlopen() use reference-counting > to avoid problems when two modules both use a particular library, but > dbus_shutdown() doesn't (and can't, really - there's no single entry point). Not sure what you means with "no single entry point". At least on windows there is dllmain (http://msdn.microsoft.com/de-de/library/windows/desktop/ms682583%28v=vs.85%29.aspx) with the reason parameter or the global c++ object of which the constructor is called on loading and the destructor on dll unloading so a single dll entry point is available or do i missed something ? What should be called else ? > The reliable and portable thing to do is to divide libraries into two > categories: "real libraries" (like libdbus, GLib, libc, libQtCore) which you > link with -ldbus-1 or whatever, and plugins which you dlopen() or > LoadLibrary(). Loading "real libraries" with dlopen() is not portable > (although it works fine on at least Linux, and the Windows equivalent with > LoadLibrary() is also OK). and is used by QtDBus (see http://qt.gitorious.org/qt/qt/blobs/4.8/src/dbus/qdbus_symbols.cpp) and QLibrary implementation (http://qt.gitorious.org/qt/qt/blobs/4.8/src/corelib/plugin/qlibrary_win.cpp). Destroying a QLibrary object will unload the library, so unloading should be supported. Unloading may also make sense for mobile applications, where services may be disabled to save resources. > When using GNU ld, it's possible to mark a library as impossible to unload, > so dlclose() does nothing (ld -z nodelete, or more commonly $(CC) > -Wl,-z,nodelete). I previously argued that plugins that are linked to > libdbus should mark *themselves* as "nodelete", since the plugin is the > thing using unusual library loading (which has the effect of preventing > libdbus from being unloaded too, since the plugin holds a reference), but > perhaps I was wrong and libdbus should be built with nodelete after all. > > Does MSVC have an equivalent of linking with the nodelete option? I never heard about related support for msvc.
(In reply to comment #70) > Should a dynamical loaded library not free all allocated memory on unload ? A library should not be unloaded, unless it's at program exit. In which case, it's not a leak, since the program is exiting. > and is used by QtDBus (see > http://qt.gitorious.org/qt/qt/blobs/4.8/src/dbus/qdbus_symbols.cpp) and > QLibrary implementation > (http://qt.gitorious.org/qt/qt/blobs/4.8/src/corelib/plugin/qlibrary_win. > cpp). Destroying a QLibrary object will unload the library, so unloading > should be supported. That's because I can't completely ban unloading in Qt, but I can and do recommend against unloading libraries and plugins. > Unloading may also make sense for mobile applications, where services may be > disabled to save resources. Unlikely to make any useful savings. The .data and .bss sections of plugins are usually just one 4k page, and that would be the saving. The majority of the resources consumed by plugins would probably be in the heap. Therefore, it's not the unloading that saves resources, it's the plugin freeing heap resources. > > When using GNU ld, it's possible to mark a library as impossible to unload, > > so dlclose() does nothing (ld -z nodelete, or more commonly $(CC) > > -Wl,-z,nodelete). I previously argued that plugins that are linked to > > libdbus should mark *themselves* as "nodelete", since the plugin is the > > thing using unusual library loading (which has the effect of preventing > > libdbus from being unloaded too, since the plugin holds a reference), but > > perhaps I was wrong and libdbus should be built with nodelete after all. I thought it was. There was a discussion with Lennart about this back in the day. I argued against nodelete, saying that dbus_shutdown should simply be called from a destructor function.
Comment on attachment 78088 [details] [review] [4] Remove unused global mutexes for win_fds, sid_atom_cache Review of attachment 78088 [details] [review]: ----------------------------------------------------------------- look good
(In reply to comment #70) > On windows there is a concept to deal with this > running-threads-before-dynamically-load-dll issues - > [thread-local storage] I don't think thread-local storage helps us here, but the "process detach" part of DllMain() would. > Should a dynamical loaded library not free all allocated memory on unload ? Not if it's going to cause crashes in practice. Suppose you have two modules both using libdbus. This would be OK: /* module 1 */ /* operating system refcount of libdbus goes from 2 down to 1 */ FreeLibrary (libdbus_handle_1); /* module 2 */ /* OK, libdbus is still loaded */ dbus_do_things (); /* operating system refcount of libdbus goes from 1 down to 0, * libdbus is unloaded (but its global memory is leaked) */ FreeLibrary (libdbus_handle_2); but this is not OK: /* module 1 */ /* Incorrect: module 1 does not know that no other module is using * libdbus (and in fact, module 2 is still using it) */ dbus_shutdown (); /* operating system refcount of libdbus goes from 2 down to 1 */ FreeLibrary (libdbus_handle_1); /* module 2 */ /* Crash: libdbus has been shut down already */ dbus_do_things (); /* Incorrect: module 2 does not know that no other module is using * libdbus (although in fact it's true here) */ dbus_shutdown (); /* operating system refcount of libdbus goes from 1 down to 0, * libdbus is unloaded */ FreeLibrary (libdbus_handle_2); > > dbus_shutdown() doesn't (and can't, really - there's no single entry point). > > Not sure what you means with "no single entry point". What I mean is that we can't change the semantics of dbus_shutdown() from "shut down" to "remove one reference; if there are no more references left, shut down", because in order to do that, we'd have to have a way for modules to take a reference, i.e. say "I'm using libdbus, don't shut it down". We don't have that at the moment, and adding it would be an incompatible change (because existing code that didn't call it would suddenly be required to call it). (In reply to comment #71) > I thought it was. There was a discussion with Lennart about this back in the > day. I argued against nodelete, saying that dbus_shutdown should simply be > called from a destructor function. Maybe. We could use the same global C++ object on Windows, __attribute__((__destructor__)) on gcc/clang, and just leak the memory as we do now on other Unix compilers, if anyone still uses other Unix compilers (I don't really want to add the C++ dependency on non-Windows). This would have the same practical result for correct library-user code - "don't call dbus_shutdown() unless you are a D-Bus maintainer and know precisely what you're doing" - but would do a bit more work on process shutdown or library unload, in exchange for being more valgrind-clean without jumping through special hoops. I certainly don't think it should block fixing this bug.
(In reply to comment #72) > Comment on attachment 78088 [details] [review] > [4] Remove unused global mutexes for win_fds, sid_atom_cache [...] > look good Merged for 1.7.4, thanks. Getting there... Thiago, you understand odd corners of Unix. Any chance you could have a look at the unmerged patches (already reviewed by Alban, who isn't a D-Bus committer), and/or skim-read the merged ones to make sure Alban, Ralf and I aren't missing anything important?
Comment on attachment 78089 [details] [review] [5] Turn a runtime assertion into a compile-time assertion has been committed to master
Comment on attachment 78088 [details] [review] [4] Remove unused global mutexes for win_fds, sid_atom_cache has been committed to master
Comment on attachment 78097 [details] [review] [11] [untested] add an automatically-initialized implementation of _dbus_lock on Windows Review of attachment 78097 [details] [review]: ----------------------------------------------------------------- see appended comment otherwise looks good ::: dbus/dbus-threads.c @@ +283,4 @@ > _dbus_platform_condvar_wake_one (cond); > } > > +#if defined(DBUS_HAVE_STATIC_RECURSIVE_MUTEXES) || defined(DBUS_WIN) init_global_locks() is defined as dummy on windows, although global locks has to be initialized and is currently performed in _dbus_threads_windows_init_global(). Would it not be cleaner to initialize global locks in a windows variant of init_global_locks() instead ?
(In reply to comment #77) > init_global_locks() is defined as dummy on windows ... and also on GNU libc ("GNU/anything"), if using patch 10. > although global locks > has to be initialized and is currently performed in > _dbus_threads_windows_init_global(). _dbus_threads_windows_init_global() is called automatically from the global C++ object's constructor, because it can't fail (it returns void and does not call malloc). The "other platforms" implementation of init_global_locks() (used on non-GNU Unix) is called from dbus_threads_init_default(). It is unsuitable to be called from the global C++ object's constructor on Windows, because it can fail on out-of-memory, and static C++ constructors don't have any way to signal failure (except by throwing an exception, but libdbus doesn't use C++ exceptions and I'm not going to start now!). > Would it not be cleaner to initialize > global locks in a windows variant of init_global_locks() instead ? If we did that, there would be no advantage in using CriticalSection: we'd still be initializing the on the first call to dbus_threads_init_default(), like we do on non-GNU Unix. The idea of this patch is to do the initialization even earlier, so that by the time Windows' DLL-loading mutex gets released, we are already as thread-safe as possible.
(In reply to comment #78) > (In reply to comment #77) > > init_global_locks() is defined as dummy on windows > > ... and also on GNU libc ("GNU/anything"), if using patch 10. I see > > > although global locks > > has to be initialized and is currently performed in > > _dbus_threads_windows_init_global(). > > _dbus_threads_windows_init_global() is called automatically from the global > C++ object's constructor, because it can't fail (it returns void and does > not call malloc). > > The "other platforms" implementation of init_global_locks() (used on non-GNU > Unix) is called from dbus_threads_init_default(). It is unsuitable to be > called from the global C++ object's constructor on Windows, because it can > fail on out-of-memory, and static C++ constructors don't have any way to > signal failure (except by throwing an exception, but libdbus doesn't use C++ > exceptions and I'm not going to start now!). > > > Would it not be cleaner to initialize > > global locks in a windows variant of init_global_locks() instead ? > > If we did that, there would be no advantage in using CriticalSection: we'd > still be initializing the on the first call to dbus_threads_init_default(), > like we do on non-GNU Unix. The idea of this patch is to do the > initialization even earlier, so that by the time Windows' DLL-loading mutex > gets released, we are already as thread-safe as possible. That means with patch 10 thread initialisation setup depends on the platform in the following manner: gnu/libc: - global locks on compile time (no initialisation required, no malloc required) - remaining thread initialisation on demand [1] windows: - global locks in dll initialisation code (c++ constructor, no malloc required) - remaining thread initialisation on demand [1] non gnu/libc: - complete thread initialisation on demand [1] [1] for example with the first call to _dbus_rmutex_new_at_location()
(In reply to comment #79) > That means with patch 10 thread initialisation setup depends on the platform > in the following manner: > > gnu/libc: > - global locks on compile time (no initialisation required, no malloc > required) > - remaining thread initialisation on demand [1] > > windows: > - global locks in dll initialisation code (c++ constructor, no malloc > required) > - remaining thread initialisation on demand [1] > > non gnu/libc: > - complete thread initialisation on demand [1] With patches 10 and 11, yes, that is correct. Before patch 11, Windows would use the "non-GNU" code path; before patch 10, GNU libc uses that code path too. I deliberately didn't implement the GNU fast-path immediately, so that the fallback path would get some testing :-) Ideally we'd use something like the GNU code path everywhere, but that doesn't seem to be possible.
Comment on attachment 78091 [details] [review] [6] Replace individual global-lock variables with an array of DBusRMutex * Review of attachment 78091 [details] [review]: ----------------------------------------------------------------- Patch seems reasonable, although I have no idea how to test this. Is there a threading test case in dbus-test, which would detect implementation problems ? ::: cmake/Doxyfile.cmake @@ -147,4 @@ > "DBUS_END_DECLS=" \ > "DOXYGEN_SHOULD_SKIP_THIS" \ > "DBUS_GNUC_DEPRECATED=" \ > - "_DBUS_DEFINE_GLOBAL_LOCK(name)=" \ probably off-topic: cmake could also use DoxyFile.in
(In reply to comment #81) > Patch seems reasonable, although I have no idea how to test this. > Is there a threading test case in dbus-test, which would detect > implementation problems ? Unfortunately, no. A test for this would have to start lots of threads, delay a random amount in each thread (?), then "do stuff with D-Bus". A multi-threaded application I was working on crashed about half the time without this patch series, and doesn't crash with it...
(In reply to comment #82) > Unfortunately, no. A test for this would have to start lots of threads, > delay a random amount in each thread (?), then "do stuff with D-Bus". > > A multi-threaded application I was working on crashed about half the time > without this patch series, and doesn't crash with it... Would valgrind's helgrind tool be useful for testing?
(In reply to comment #83) > Would valgrind's helgrind tool be useful for testing? If you've created a non-thread-safe situation, helgrind is useful for debugging that situation; but as far as I'm aware, it isn't useful to set up the non-thread-safe situation in the first place. The situation most likely to trigger bugs in this set of patches would be if a process that has not previously used libdbus APIs starts multiple threads, then two or more of those threads on different CPU cores start using libdbus at the same moment.
I've had positive non-D-Bus-maintainer reviews on this stuff (from Alban Crequy and Anas Nashif) and no actual negative reviews or vetoes, and it's had some testing in a patched libdbus, so I'm going to merge it and hope for the best. It certainly fixes some observed crashes (when the patch series was accidentally reverted, application crashes came back). I'm not going to merge patch 11 without an ack from Ralf (or someone else who can test it on Windows), but hopefully that patch is OK too.
Comment on attachment 78091 [details] [review] [6] Replace individual global-lock variables with an array of DBusRMutex * merged
Comment on attachment 78094 [details] [review] [8] Make taking a global lock automatically initialize locking if needed committed
Comment on attachment 78095 [details] [review] [9] Always initialize threading before allocating a dynamic mutex pushed
Comment on attachment 78096 [details] [review] [10] Add a statically-initialized implementation of _dbus_lock() on glibc systems applied
(In reply to comment #55) > [12] Stop asserting that we're not using the dummy lock implementation > > That implementation no longer exists, so neither 0xABCDEF nor 0xABCDEF2 > has any special meaning any more. > > --- > > Alban suggested this. I agree it makes sense. Alban, any chance you could review this?
Comment on attachment 79028 [details] [review] [12] Stop asserting that we're not using the dummy lock implementation The patch looks good to me!
(In reply to comment #89) > [10] Add a statically-initialized implementation of _dbus_lock() on glibc > systems This wasn't actually right: it produced different results, depending whether pthread.h had been included. I reverted it.
*** Bug 40072 has been marked as a duplicate of this bug. ***
Comment on attachment 79028 [details] [review] [12] Stop asserting that we're not using the dummy lock implementation Applied for 1.9.2
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/73.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.