The documentation was fairly clear before, but we can make it clearer:
it’s a programming error to call `g_main_context_release()` if you have
not received a true return value from `g_main_context_acquire()` before.
Inspired by https://gitlab.gnome.org/GNOME/glib/-/merge_requests/3302.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
A context iteration we're doing lots of lock/unlocks and that's fine to give
other threads contexts a chance to run, but we're doing it also just to call
other functions that require locking, and this can be avoided.
Other threads can still have a chance to run while releasing the ownership
of the context.
musl doesn’t define them itself, presumably because they’re not defined
in POSIX. glibc does define them. Thankfully, the values used in glibc
match the values used internally in other musl macros.
Define the values as a fallback. As a result of this, we can get rid of
the `g_assert_if_reached()` checks in `siginfo_t_to_wait_status()`.
This should fix catching signals from a subprocess when built against
musl.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Fixes: #2852
A file-descriptor was created with the introduction of pidfd_getfd() but
nothing is closing it when the source finalizes. The GChildWatchSource is
the creator and consumer of this FD and therefore responsible for closing
it on finalization.
The pidfd leak was introduced in !2408.
This fixes issues with Builder where anon_inode:[pidfd] exhaust the
available FD limit for the process.
Fixes#2708
When the system supports it (as all Linux kernels ≥ 5.3 should), it’s
preferable to use `pidfd_open()` and `waitid()` to be notified of
child processes exiting or being signalled, rather than installing a
default `SIGCHLD` handler.
A default `SIGCHLD` handler is global, and can never interact well with
other code (from the application or other libraries) which also wants to
install a `SIGCHLD` handler.
This use of `pidfd_open()` is racy (the PID may be reused between
`g_child_watch_source_new()` being called and `pidfd_open()` being
called), so it doesn’t improve behaviour there. For that, we’d need
continuous use of pidfds throughout GLib, from fork/spawn time until
here. See #1866 for that.
The use of `waitid()` to get the process exit status could be expanded
in future to also work for stopped or continued processes (as per #175)
by adding `WSTOPPED | WCONTINUED` into the flags. That’s a behaviour
change which is outside the strict scope of adding pidfd support,
though.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #1866Fixes: #2216
Do not wakeup immediately for our own context wakeup poll registration.
g_main_context_add_poll_unlocked() will call g_wakeup_signal() to wake
up the loop waiting in poll(). Doing so during context creation will
create an extra wakeup for the first poll().
Skip the wakeup call if it is the wake_up_rec registration. No other
sources/caller should ever reach that condition.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Instead store a bit inside `GTimeoutSource` and `GIdleSource` to
indicate that they are one-shot sources, and that their callbacks have a
different type and should always be assumed to return `G_SOURCE_REMOVE`.
This should make one-shot idle and timeout sources a teeny weeny little
bit cheaper to set up.
From a suggestion here: https://gitlab.gnome.org/GNOME/glib/-/merge_requests/2684#note_1462917
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
This allows it to be reused and extended (internally) a little more.
This commit introduces no functional changes, but allows for more easy
additions in a following commit.
It introduces `GIdleSource` as a simple wrapper around `GSource`, which
will be extended in a following commit.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
This allows it to be reused and extended (internally) a little more.
This commit introduces no functional changes, but allows for more easy
additions in a following commit.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Many idle and timeout sources are installed as "one shot": called once
and immediately removed. While it's easy to write a simple callback that
returns G_SOURCE_REMOVE, it would also be useful to have some sort of
"visual" marker when reading the code; a way to immediately see that a
callback (which may be defined elsewhere in the code) is meant to be
invoked just once.
Includes additional unit tests by Philip Withnall.
Add SPDX license (but not copyright) headers to all files which follow a
certain pattern in their existing non-machine-readable header comment.
This commit was entirely generated using the command:
```
git ls-files glib/*.[ch] | xargs perl -0777 -pi -e 's/\n \*\n \* This library is free software; you can redistribute it and\/or\n \* modify it under the terms of the GNU Lesser General Public/\n \*\n \* SPDX-License-Identifier: LGPL-2.1-or-later\n \*\n \* This library is free software; you can redistribute it and\/or\n \* modify it under the terms of the GNU Lesser General Public/igs'
```
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #1415
This fixes a scan-build warning:
```
../../../../source/glib/glib/gmain.c:4193:18: warning: 2nd function call argument is an uninitialized value [core.CallAndMessage]
while ((nfds = g_main_context_query (context, max_priority, &timeout, fds,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```
This is a valid situation which can occur if the preceding
`g_main_context_prepare()` call returns `FALSE` and doesn’t set
`max_priority`.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #1767
When calling `g_main_loop_run()` it’s possible for a reference to the
`GMainLoop` to leak if this thread had to wait to acquire ownership of
the `GMainContext`.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Fixes: #2598
This comment was added in a919be3d in 2013. The function basically looked the same
as now at that point. g_main_context_find_source_by_id() can clearly return NULL if
the source id is invalid, so unclear where this comes from? The function in
question are approximately the same since e2fd4e2b in 2000.
However back then g_main_context_find_source_by_id() would actually always return
the last source if there was none with the given source id (wat, that's clearly
unintended?). This was changed in 393503ba in 2014, arguably an API change of that
function but more arguably a bugfix :)
So for a short time between 2013 and 2014, that comment was correct. Now it is not
anymore and can be removed.
It fixes a race. g_source_attach() had the following check to ensure a
loop blocked on poll() would wakeup.
if (do_wakeup && context->owner && context->owner != G_THREAD_SELF)
g_wakeup_signal (context->wakeup);
However it doesn't contemplate an implementation where poll()ing is a
non-blocking operation that will be scheduled while the thread is
released to perform other tasks. This scenario opens up several
different possibilities where the condition would fail to hold true. I
experienced two of such races.
The first race pertains to a mono-threaded application. Do keep in mind
that integrating GLib to a foreign event loop will make GLib act as a
slave in the new event loop. When you post a new work unit to execute in
the thread managed by the foreign event loop, you don't use
g_main_context_invoke(). In fact the only reason to integrate
GMainContext in a foreign event loop is to make the two of them
communicate. So from time to time, the foreign event loop will execute
callbacks that manipulate the GMainContext loop. An illustration
follows.
// in this callback we translate an event from the foreign event loop
// to an event in the GMainContext event loop (that runs in the same
// thread)
static void my_event_loop_callback(void* data)
{
GMainContext* ctx = /* ... */;
// ...
g_source_attach(source, ctx);
}
int main()
{
// ...
my_event_loop_invoke(my_event_loop_callback, data);
// ...
// this function has all mechanisms in place to run the foreign
// event loop and the hooks to call
// g_main_context_{prepare,query,check,dispatch}
my_event_loop_run();
}
In this case, you would have the following series of calls:
1. g_main_context_prepare()
2. g_main_context_query()
3. A callback to my_event_loop is registered when any fd on the set is
ready or the timeout is reached.
4. The thread is released to perform other tasks.
5. One of the tasks executed wishes to communicate with my_event_loop
and enters my_event_loop_callback.
6. g_source_attach() is called.
7. g_source_attach() detects do_wakeup=TRUE, context->owner != NULL, and
context->owner == G_THREAD_SELF so g_wakeup_signal() is skipped.
8. None of the fds on the GLib poll() set becomes ready nor the GLib
timeout expires. The my_event_loop callback that would call
g_main_context_check() is never executed. Deadlock.
A shallow analysis will fail to detect the race here. The explanation
seems to showcase a scenario that will deterministically fail with a
deadlock every time. However do keep in mind that my_event_loop_callback
could be invoked before or after g_main_context_prepare(). There is an
_event_ race here. Furthermore, some GLib libraries such as GDBus will
initialize objects from extra threads (GAsyncInitable interface) and
invoke the result on the original thread when ready (g_source_attach()
will eventually be called). Now you have scenarios closer to standard
race examples.
The other scenario where a race would manifest happens in a
multi-threaded application that has a concurrency design similar to the
actor model. No actor executes in two threads simultaneously, but it's
not guaranteed that it'll always wake-up in the same thread. It'd
perform steps 1-4 just as in the previous example, but before thread
control is returned to the pool, it'd call g_main_context_release(). Now
g_source_attach() would skip g_wakeup_signal() for a different reason:
7. g_source_attach() detects do_wakeup=TRUE, context->owner == NULL so
g_wakeup_signal() is skipped.
8. Same as before.
Certainly there are other concurrency designs where this optimization
would cause a deadlock, but all of them have origin in the same place:
the optimization assumes the poll() implementation is a blocking
operation and the thread will never be released to perform other tasks
(possibly involving GLib calls) while result is not ready. They share
not only the same problem, but also the same solution: do not make
assumptions and just call g_wakeup_signal().
This patch implements this solution by introducing the
G_MAIN_CONTEXT_FLAGS_OWNERLESS_POLLING flag. This flag will force a call
to g_wakeup_signal() and fix the race on foreign event loops. The reason
to prevent changing this option after creation is to avoid other races
that would lead to event loss. Construction is the only proper time to
set this option.
The implementation design means we do not change **any** semantics for
current working code. If you don't set the new flag, the code won't
enter in different branches and current behavior won't be affected. The
patch is small and easy to follow too.
g_source_set_name duplicates the string, and this is
showing up as one of the more prominent sources of strdups
in GTK profiles, despite all the names we use being literals.
Add a variant that avoids the overhead.
On Unix platforms, wait() and friends yield an integer that encodes
how the process exited. Confusingly, this is usually not the same as
the integer passed to exit() or returned from main(): conceptually it's
an integer encoding of this tagged union:
enum { EXITED, SIGNALLED, ... } tag;
union {
int exit_status; /* if EXITED */
struct {
int terminating_signal;
bool core_dumped;
} terminating_signal; /* if SIGNALLED */
...
} detail;
Meanwhile, on Windows, wait statuses and exit statuses are
interchangeable.
I find that it's clearer what is going on if we are consistent about
referring to the result of wait() as a "wait status", and the value
passed to exit() as an "exit status".
GSubprocess already gets this right: g_subprocess_get_status() returns
the wait status, while g_subprocess_get_exit_status() genuinely returns
the exit status. However, the GSpawn family of APIs has tended to
conflate the two.
Confusingly, g_spawn_check_exit_status() has always checked a wait
status, and it would not be correct to pass an exit status to it; so
let's deprecate it in favour of g_spawn_check_wait_status(), which
does the same thing that g_spawn_check_exit_status() always did.
Code that needs backwards-compatibility with older GLib can use:
#if !GLIB_CHECK_VERSION(2, 69, 0)
#define g_spawn_check_wait_status(x) (g_spawn_check_exit_status (x))
#endif
Signed-off-by: Simon McVittie <smcv@collabora.com>
This seems non-obvious to me. Document it.
It also seems important to know, because it means that the data pointer
might already be destroyed, before the source is unreferenced for good.
For example, if the data pointer keeps a reference to the GSource,
and it might seem sensible to call:
g_source_destroy(data->source);
g_source_unref(data->source); /* <<< data is already destroyed? */
This leads to a crash, if the source was attached to a context.
This is basically glnx_steal_fd() from libglnx. We already had two
private implementations of it in GLib.
Signed-off-by: Simon McVittie <smcv@collabora.com>
These variables were already (correctly) accessed atomically. The
`volatile` qualifier doesn’t help with that.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #600
If there is a file descriptor source that has a lower priority
than the one for sources that are going to be dispatched,
all subsequent file descriptor sources (internally sorted by
file descriptor identifier) do not get an update in their GPollRec
and later on wrong sources can be dispatched.
Fix this by first finding the first GPollRec that matches the current
GPollFD, instead of relying on it to be the current one. At
the same time, document the assumptions about the ordering of the
file descriptor records and array and make explicit in the documentation
that the array needs to be passed to g_main_context_check() as it was
received from g_main_context_query().
Added a new test that reproduces the bug by creating two file
descriptor sources and an idle one. Since the first
file descriptor created has a lower identifier and a low priority,
the second one is not dispatched even when it has the same, higher,
priority as the idle source. After fixing this bug, both
higher priority sources are dispatched as expected.
While this patch was written independently, a similar fix for this
bug was first submitted by Eugene M in GNOME/glib!562. Having a
second fix that basically does the same is a reassurance that we
are in the right here.
Fixes#1592
When unref'ing child sources, the lock is already held. But instead of
passing TRUE to g_source_unref_internal it currently passes whether the
lock was already held outside of the current invocation. Just pass TRUE
to fix this possible issue.
`getenv()` doesn't work well on Windows, f.ex., it can't fetch env
vars set with `SetEnvironmentVariable()`. This also means that it
doesn't work at all when targeting UWP since that's the only way to
set env vars in that case.
This allows you to see how long each `GMainContext` iteration and each
`GSource` `check`/`prepare`/`dispatch` takes. It provides more detail
than sysprof’s speedtrack plugin can provide, since it has access to
more internal GLib data.
Use it with `sysprof-cli`, for example:
```
sysprof-cli --use-trace-fd -- my-test-program
```
Signed-off-by: Philip Withnall <withnall@endlessm.com>
It seems that `sig_atomic_t` is not the same width as `int` on FreeBSD,
which is causing CI failures:
```
../glib/gmain.c:5206:3: error: '_GStaticAssertCompileTimeAssertion_73' declared as an array with a negative size
g_atomic_int_set (&any_unix_signal_pending, 0);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../glib/gatomic.h💯5: note: expanded from macro 'g_atomic_int_set'
G_STATIC_ASSERT (sizeof *(atomic) == sizeof (gint)); \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```
Fix that by only using `sig_atomic_t` if the code is *not* using atomic
primitives (i.e. in the fallback case). `sig_atomic_t` is only a typedef
around an integer type and is not magic. Its typedef is chosen by the
platform to be async-signal-safe (i.e. read or written in one instruction),
but not necessarily thread-safe.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
