The thread pool test is currently flakey and takes over 2 minutes to run
on CI. It also currently has no users now that RequestServer uses curl,
so let's just remove it for now. If we need it in the future, we can
revive it from git history.
The `ThreadPoolLooper` should increment `m_busy_count` before attempting
to access the global queue. Otherwise, there exists a possible race
condition where `wait_for_all` checks the exit conditions before the
looper increments `m_busy_count` but after it empties the `ThreadPool`
queue.
Next, incrementing / decrementing `m_busy_count` is moved to be the
responsibility of `ThreadPoolLooper`. Otherwise, it is possible that
decrementing `m_busy_count` in the caller of `Looper::next` causes
`m_busy_count` to underflow if the call to `Looper::next` returns
before incrementing `m_busy_count`.
When adding tests for `ThreadPool` a handful of deadlocks can be
observed when worker threads wait on `m_work_available`.
The first deadlock is in the destruction of `ThreadPool` where it
is possible for a worker thread to be in the process of acquiring
`m_mutex` when the broadcast to `m_work_available` in the
destructor happens. This causes the destructor to hang on joining the
thread which is now perpetually waiting on `m_work_available`. This is
solved by repeatedly broadcasting on `m_work_available` until the thread
to join exits.
The second deadlock occurs when the final signal to `m_work_done` is
missed by the wait in `wait_for_all`. At this point all workers are in
the hot loop of attempting to get work from the work queue, however
since there is no work remaining all workers end up waiting on
`m_work_available`. At this point the `wait_for_all` call is also
waiting on `m_work_done`, which will never be signalled again as all
workers are waiting on `m_work_available`.
This requires 2 changes to fix, the first is that workers will signal
`m_done_work` before waiting on `m_work_available`. The second change is
to acquire `m_mutex` before checking the wait conditions as done when
using `wait_while`.
This commit un-deprecates DeprecatedString, and repurposes it as a byte
string.
As the null state has already been removed, there are no other
particularly hairy blockers in repurposing this type as a byte string
(what it _really_ is).
This commit is auto-generated:
$ xs=$(ack -l \bDeprecatedString\b\|deprecated_string AK Userland \
Meta Ports Ladybird Tests Kernel)
$ perl -pie 's/\bDeprecatedString\b/ByteString/g;
s/deprecated_string/byte_string/g' $xs
$ clang-format --style=file -i \
$(git diff --name-only | grep \.cpp\|\.h)
$ gn format $(git ls-files '*.gn' '*.gni')
Previously, a `Thread` could be deleted while its action was running,
even if it was running detached.
By changing it to be atomically reference counted, and incrementing the
count when starting the action, we can keep the Thread and its running
action `Function` alive until it exits. Thus, detached `Thread` objects
can be deleted by the thread creating them and allowed to die
naturally.
Inheritance from `EventReceiver` on the `Thread` class was only used in
the `BackgroundAction` class, where the children vector was keeping the
action alive until the work was completed. However, this can be
accomplished by instead capturing a `NonnullRefPtr` of `this`. The work
function can then avoid having to remove the `BackgroundAction` from
its parent `Thread` when the work completes.
Since Core::Object properties are really only used by GML now that the
Inspector is long gone, there's no need for these to pollute
Core::Object.
This patch adds a new GUI::Object class to hold properties, and makes
it the new base class of GUI::Window, GUI::Widget and GUI::Layout.
The "instantiate an object by name" mechanism that GML uses is also
hoisted into GUI::Object as well.
The previous iteration of this API was somewhat odd and rough in random
places, which degraded usability and made less than perfect sense.
This commit reworks the API to be a little closer to more
conventional promise APIs (a la javascript promises).
Also adds a test to ensure the class even works.
Contrary to the log message that was shown, it is not decidedly a logic
error to detach a thread that has exited. Even in the simple unit tests
we have, it's possibly for the thread to run and exit in between the
time it takes for our call to Thread::start() to complete and
Thread::detach() to run. This is often seen on CI.
This class can be used to run a task in another thread, and allows the
caller to wait for the task to complete to retrieve any error that may
have occurred.
Currently, it doesn't support functions returning a value on success,
but with some template magic that should be possible. :^)
This change ensures that a reference to the BackgroundAction is held
until after the error callback has executed on the event loop.
This fixes an intermittent crash in Assistant :^)
As the user might want to have interactions with LibGUI while handling
errors, this code should be executed in the original `EventLoop`.
Similarly to what is done with the error-free path.
BackgroundActions are now added as a job to the event loop, therefore
they get canceled when the loop exits and their on_complete action never
runs. This fixes all UAF bugs related to BackgroundAction's use of
EventLoops, as seen with e.g. thumbnail generation.
These instances were detected by searching for files that include
AK/Format.h, but don't match the regex:
\\b(CheckedFormatString|critical_dmesgln|dbgln|dbgln_if|dmesgln|FormatBu
ilder|__FormatIfSupported|FormatIfSupported|FormatParser|FormatString|Fo
rmattable|Formatter|__format_value|HasFormatter|max_format_arguments|out
|outln|set_debug_enabled|StandardFormatter|TypeErasedFormatParams|TypeEr
asedParameter|VariadicFormatParams|v_critical_dmesgln|vdbgln|vdmesgln|vf
ormat|vout|warn|warnln|warnln_if)\\b
(Without the linebreaks.)
This regex is pessimistic, so there might be more files that don't
actually use any formatting functions.
Observe that this revealed that Userland/Libraries/LibC/signal.cpp is
missing an include.
In theory, one might use LibCPP to detect things like this
automatically, but let's do this one step after another.
This file is not needed here, and causes a compile issue on musl-libc
based distributions. We should only be including this file in LibC, for
the most part anyway.
This patch allows returning an `Error` from the `on_complete` callback
in `BackgroundAction`.
It also adds a custom callback to manage errors returned during its
execution.
This replaces all state-related variables with a single ThreadState.
These are simplified over what the Kernel has, but capture all
userspace-available thread state.
Locking the state behind an atomic and using proper atomic operations
also gets rid of quite some deadlocks and race conditions that have
existed around m_tid and others beforehand.
In terms of behavior, this introduces the following changes:
- All thread state mishandling (e.g. joining a detached thread) crashes
the program. Mishandling thread state is a severe kind of concurrency
bug that might also be indeterministic, so letting it silently
disappear with the return value of pthread_ APIs is a bad idea. The
thread state can always be checked beforehand to ensure that no crash
happens.
- Destructing a still-running thread will crash in AK/Function, so the
Thread destructor issues its own warning for debugging purposes.
- Thread issues warnings before crashes in many places to aid
concurrency debugging (the most difficult kind of debugging).
- Joining dead but not detached threads is legal, as per POSIX APIs.
- The thread ID is never reset to 0 after the thread has been started
and subsequently been assigned a valid thread ID. The thread's exit
state is still obtainable.
- Detaching threads that are about to exit is considered a programming
bug and will often (not always, as we can't catch all execution
sequences involved in such a situation) crash the program on purpose.
If you want to detach a thread that will definitely exit on its own,
you have to prevent it from exiting before detach() was called (e.g.
with an "exit requested" flag).
We have a new, improved string type coming up in AK (OOM aware, no null
state), and while it's going to use UTF-8, the name UTF8String is a
mouthful - so let's free up the String name by renaming the existing
class.
Making the old one have an annoying name will hopefully also help with
quick adoption :^)
Otherwise, we end up propagating those dependencies into targets that
link against that library, which creates unnecessary link-time
dependencies.
Also included are changes to readd now missing dependencies to tools
that actually need them.
Even though the toolchain implicitly links against -lc, it does not know
where it should get LibC from except for the sysroot. In the case of
Clang this causes it to pick up the LibC stub instead, which might be
slightly outdated and feature missing symbols.
This is currently not an issue that manifests because we pass through
the dependency on LibC and other libraries by accident, which causes
CMake to link against the LibC target (instead of just the library),
and thus points the linker at the build output directory.
Since we are looking to fix that in the upcoming commits, let's make
sure that everything will still be able to find the proper LibC first.
Previously, init() in BackgroundAction.cpp was calling
Core::Object::set_name, which does not affect the displayed thread name
which is displayed by the system monitor. This changes it to pass the
name to the thread constructor.
pthread_setname_np is a can of worms for portability. Linux, macOS,
and the BSDs all do it differently.
Also skip adding the tid as an inspectable Core::Object property on
systems where pthread_t is known to be a pointer.
Users can now determine whether a thread has been started or not. A
started thread might also have already terminated.
Implementation note: We *could* detect this with pthread APIs maybe, but
this is much simpler.
While null StringViews are just as bad, these prevent the removal of
StringView(char const*) as that constructor accepts a nullptr.
No functional changes.
Each of these strings would previously rely on StringView's char const*
constructor overload, which would call __builtin_strlen on the string.
Since we now have operator ""sv, we can replace these with much simpler
versions. This opens the door to being able to remove
StringView(char const*).
No functional changes.
Because the wake pipe is thread-local, it was previously not possible
to wake an event loop across a thread. Therefore, this commit
rearchitects event loop waking by making the wake function a member of
the event loop itself and having it keep a pointer to its thread's wake
pipe. The global wake() function calls wake on the current thread's
event loop.
This also fixes a bug in BackgroundAction: it should wake the event loop
it was created on, instead of the current thread's event loop.
We now capture the origin thread's current event loop when setting up
a BackgroundAction and then invoke the on_complete callback on that same
event loop.
MutexProtected mirrors the identically-named Kernel primitive and can be
used to synchronize access to any object that might not be thread safe
on its own. Synchronization is done with a simple mutex, so access to a
MutexProtected object is potentially blocking.
Mutex now has an internal nesting variable which is there to harden it
against lock-unlock ordering issues (e.g. double unlocking).
