This repository was archived by the owner on Apr 28, 2020. It is now read-only.
T29303 Thread pool fix 2 #75
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
g_assert() can be compiled out with G_DISABLE_ASSERT, which renders the test rather useless. Signed-off-by: Philip Withnall <withnall@endlessm.com> https://gitlab.gnome.org/GNOME/glib/issues/1515
These checks used to be a precondition on test_threaded_singleton(); but the earlier tests could leave the refcount of the shared connection in a bad state, and this wouldn’t be caught until later. Factor out the check, increase the iteration count to 1000 (so the check blocks for up to 1s rather than 100ms), and call it in more places. Signed-off-by: Philip Withnall <withnall@endlessm.com> https://gitlab.gnome.org/GNOME/glib/issues/1515
The fix for bgo#651133 (commit 7e0f890) introduced a kind of weak ref, which had to be thread-safe due to the fact that `GDBusProxy` operates in one thread but can emit signals in another. Since that commit, `GWeakRef` was added, which does the same thing. Drop the custom code in favour of it; this should be functionally equivalent, but using an RW lock rather than a basic mutex, which should reduce contention. Signed-off-by: Philip Withnall <withnall@endlessm.com>
Use `g_clear_object()` to tidy things up. This introduces no functional changes. Signed-off-by: Philip Withnall <withnall@endlessm.com>
This introduces no functional changes. Signed-off-by: Philip Withnall <withnall@endlessm.com>
`CallDestroyNotifyData` never uses that `GMainContext`, and holding a ref to it could cause reference count cycles if the `GMainContext` is no longer being iterated. Signed-off-by: Philip Withnall <withnall@endlessm.com> Helps: #1515
This removes an unhelpful `goto`. It introduces no functional changes. Signed-off-by: Philip Withnall <withnall@endlessm.com>
This introduces no functional changes, but makes the code a little more explicit about which connection and main context it’s operating on. Signed-off-by: Philip Withnall <withnall@endlessm.com> Helps: #1515
Iterate the given `context` while waiting, rather than sleeping. This ensures that if the errant `GDBusConnection` ref is held by some pending callback in the given `context`, it will actually be released. Typically `context` is going to be the global default main context. Signed-off-by: Philip Withnall <withnall@endlessm.com> Helps: #1515
Previously, the code in `ensure_gdbus_testserver_up()` created a proxy object and watched its `name-owner` to see when the `com.example.TestService` name appeared. This ended up subscribing to three signals (one of them for name ownership, and two unused for properties of the proxy), and was racy. In particular, the `name-owner` property could be set before all D-Bus messages had been processed — it could have been derived from getting the owner of the name, for example. This left unprocessed messages hanging around in the `context`, but that context was never iterated again, which essentially leaked the references held by those messages. That included a reference to the `GDBusConnection`. The first part of the fix is to simplify the code to use `g_bus_watch_name_on_connection()`, so there’s only one signal subscription to worry about. The second part of the fix is to use the `GDestroyNotify` callback for the watch data to be notified of when all D-Bus traffic has been processed and the signal unsubscription is complete. At this point, it’s guaranteed that there are no idle callbacks pending in the `GMainContext`, since the `GDestroyNotify` callback is the last one invoked on the `GMainContext`. Essentially, this commit uses the `GDestroyNotify` callback as a synchronisation message between the D-Bus worker thread and the thread calling `ensure_gdbus_testserver_up()`. Signed-off-by: Philip Withnall <withnall@endlessm.com> Fixes: #1515
As with the previous commit, don’t stop iterating the `context` in `test_delivery_in_thread_func()` until the unsubscription from a signal is complete, and hence there’s a guarantee that no callbacks are pending in the `thread_context`. This commit uses the `GDestroyNotify` for `g_dbus_connection_signal_subscribe()` as a synchronisation message from the D-Bus worker thread to the `test_delivery_in_thread_func()` thread to notify of signal unsubscription. Signed-off-by: Philip Withnall <withnall@endlessm.com> Fixes: #1515
This is equivalent, but makes the loop exit conditions a little clearer, since they’re actually in a `while` statement, rather than being a `g_main_loop_quit()` call in a callback somewhere else in the file. Signed-off-by: Philip Withnall <withnall@endlessm.com> Helps: #1515
When testing that signals are delivered to the correct thread, and are delivered the correct number of times, call `EmitSignal()` on the `gdbus-testserver` to trigger a signal emission, and listen for that. Previously, the code listened for `NameOwnerChanged` and connected to the bus again to trigger emission of that. The problem with that is that other things happening on the bus (for example, an old `gdbus-testserver` instance disconnecting) can cause `NameOwnerChanged` signal emissions. Sometimes, the `gdbus-threading` test was failing the `signal_count == 1` assertion due to receiving more than one `NameOwnerChanged` emission. Signed-off-by: Philip Withnall <withnall@endlessm.com> Helps: #1515
See previous commits and #1515. Signed-off-by: Philip Withnall <withnall@endlessm.com> Fixes: #1515
Add a note to the documentation of `g_dbus_connection_signal_unsubscribe()`, `g_bus_unwatch_name()` and `g_bus_unown_name()` warning about the need to continue iterating the caller’s thread-default `GMainContext` until the unsubscribe/unwatch/unown operation is complete. See the previous few commits and #1515 for an idea of the insidious bugs that can be caused by not iterating the `GMainContext` until everything’s synchronised. Signed-off-by: Philip Withnall <withnall@endlessm.com>
bindfs is part of the setup process, so if it fails (as can happen if the `fuse` kernel module has not been loaded — not much we can do about that) then skip the test. Signed-off-by: Philip Withnall <withnall@endlessm.com>
Instead only do a g_critical(). This is something that has to be fixed one way or another, but a g_critical() is less disruptive and makes sure that code that worked in previous GLib versions still works as bad as before. Fixes https://gitlab.gnome.org/GNOME/glib/issues/2039
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Trivial backport of:
https://phabricator.endlessm.com/T29303