libretro/RetroArch
1
0 Fork 0
mirror of https://github.com/libretro/RetroArch.git synced 2026-11-07 08:36:02 -10:00
Code Issues Packages Projects Releases 1 Wiki Activity

UI non-blocking, part 3: netplay host init and content close (#19417)

Browse Source 
* netplay: bound the wait for tunnel information

Stage 1 of making netplay host init non-blocking: the hang becomes an
error, with no new states and no restructuring.

Hosting through a relay waits for the lobby server to answer with a
tunnel address.  The wait was already narrowed to that one query, but
it was still unbounded - and it is a round trip to a machine we do
not control.  A lobby server that is down, blackholed or merely slow
left the frontend hung with no way out but killing it.  It now gives
up after ten seconds and reports failure like any other query, so the
caller falls back to direct mode with the warning it already has.
Generous on purpose: timing out a query that would have succeeded
costs the user their session, while waiting a few seconds longer
costs only patience.

task_queue_wait_timeout() is the bound.  It wraps the caller's
condition in one that also goes false at a deadline and hands that to
the existing wait, so every queue implementation - regular, threaded
and GCD - keeps its own waiting behaviour and no platform-specific
code is touched.  Adding a variant to the implementation vtable would
have meant editing the GCD path blind, which is the shape of a recent
break and not worth repeating for this.

A timeout alone would have been a bug, not a fix.  The query is still
in flight when we give up, and its callback writes
host_room->mitm_address and mitm_port unconditionally - so it would
land after the fallback and publish a tunnel address for a session
running direct.  Each query now carries a generation token and the
callback ignores results that are no longer current; abandoning a
query bumps the generation.  That also closes the pre-existing case
of two queries overlapping.

The oracle covers the primitive rather than netplay's HTTP, because
that is where the risk is: the netplay side is a few lines routing
into an else branch that already existed.  Five lanes over both queue
implementations assert that a satisfiable wait returns as soon as it
is satisfied - not after sitting out the timeout, which would pass a
naive check - and that an unsatisfiable one gives up near the
deadline rather than early or never.  Removing the deadline hangs the
lane until CI kills it, which is the behaviour being removed.

Verified: builds with networking ENABLED and DISABLED, the latter
being the configuration class that broke recently; task_queue.c links
for ARM32 without NEON; all nine task oracles still pass, since that
file is shared; the new lanes pass plain, under ASan+UBSan+LSan and
twice under TSan.

Not verified: behaviour against a real unresponsive lobby server.

* netplay: split the tunnel query into begin / await / result

Stage 2 of making netplay host init non-blocking: a refactor that
changes no behaviour, so it can be reviewed on its own before
anything moves.

netplay_mitm_query() welded "ask the lobby server" to "wait for the
answer".  Those are now netplay_mitm_query_begin(),
netplay_mitm_query_ready(), netplay_mitm_query_await() and
netplay_mitm_query_result(), with netplay_mitm_query() calling them
back to back at the same call site.  Nothing is deferred and nothing
moves; the point is that the next step - issuing the query when the
user opens the host UI, so the answer is already there when host
setup runs - becomes a change of WHERE begin() is called rather than
a restructuring of host init.

Every path was walked against the original:

  handle NULL/empty       false                 begin() false
  custom, decode fails    false                 begin() false
  custom, decode ok       address set, result   begin(), await(), result()
  http push fails         pending cleared,false begin() false
  http ok, reply in time  wait, then result     await() waits, result()
  http ok, timeout        warn, bump gen, false await() identical

One deliberate difference, because the split exposed an edge the old
shape hid.  The custom-relay path answers synchronously and the old
code returned without ever consulting the pending flag - so an
earlier query left outstanding could not make it wait.  Routed
through await() it would have, so the custom path now supersedes any
query in flight: it bumps the generation and clears the flag.  That
also stops a late callback from overwriting the address it just set,
which the old code would have allowed.

Verified: builds with networking enabled AND disabled; the
wait_timeout CI step replayed verbatim; the netplay NAT, task
ordering and watchdog oracles still pass.

Not verified: behaviour against a real lobby server.

* netplay: start the tunnel query when the user commits to hosting

Stage 3, and the one that removes the stall the user actually feels.

Host setup needs the relay address, and the query for it was issued
at the moment of need - so the wait was the full round trip, every
time.  It is now issued at CMD_EVENT_NETPLAY_ENABLE_HOST, where the
user commits to hosting, which is well before the address is wanted:
on the content-reload path, before content has even started loading.
The round trip overlaps work that was going to happen anyway, so by
the time host setup asks, the answer is usually already there and the
wait costs nothing.

Nothing is deferred and host init is not restructured.  The wait
stays exactly where it was, as the backstop for the case where the
answer has not arrived yet - which is what makes this cheap to
reason about and cheap to revert.

Prefetching only matters if asking twice is avoided, so the query
records the handle it was issued for, and host setup reuses an
answer that is already in hand or on its way.  It deliberately does
NOT reuse a prefetch that came back empty: a transient lobby failure
at prefetch time would otherwise doom a session that asking again
would have got, and one extra request is the better trade.  Every
state was walked:

  no prefetch                query issued as before
  prefetch in flight, same   reused, one query, overlapped
  prefetch answered          reused, no wait at all
  prefetch came back empty   re-issued
  relay changed since        re-issued, picks the new server
  custom relay               answered synchronously, reused

The prefetch is best effort in the strictest sense: skipped when the
relay is off or unconfigured, and if it fails for any reason host
setup asks exactly as it did before.  It is not speculative - the
user has already chosen to host, so no request is made for anyone who
merely opens a menu.

Verified: the call site sits inside #ifdef HAVE_NETWORKING, computed
from the preprocessor nesting rather than eyeballed, and both builds
were made - networking enabled AND disabled, the configuration class
that broke recently.  The wait_timeout CI step was replayed verbatim
from the workflow, and the netplay NAT and task ordering oracles
pass.

Not verified: behaviour against a real lobby server, including
whether the overlap is long enough in practice to hide the round
trip on the content-reload path.

* content close: pin the save wait, and say what it is waiting for

Stages 0 and 1 of unblocking content close.  Nothing about the wait
changes; this establishes what it guarantees and stops it looking
like a hang.

Stage 0, the oracle lane.  Closing content waits for in-flight save
and load tasks because their handlers call into core function
pointers - retro_serialize via content_get_serialized_data,
retro_unserialize via core_unserialize - while
runloop_event_deinit_core() unloads the dylib underneath them.  A
worker mid-call when the library goes away dispatches into freed
code.  That is why this wait, unlike the others removed in this
series, cannot simply be deleted.

The lane pins the property the unload depends on: the file is
checked incomplete before content_wait_for_save_state_task() and
complete after, so a wait that returned early - on a budget
boundary, say - fails here rather than becoming a crash on a device.
Neutering the wait fails it, as it should.

Stage 1, legibility.  The wait cannot be shortened without deferring
the teardown itself, which is later work, but an unexplained frozen
frame and a frame that says "saving state" are very different
experiences - and on slow storage this is seconds, not milliseconds.
The message is pushed and one frame forced out before blocking,
since nothing draws once the wait starts.

It fires only when there is something to wait for, using the same
condition the wait itself uses, so an ordinary close with no save in
flight is untouched and shows nothing.  That predicate had to become
public to be asked from here; its load counterpart already was.

Verified: builds with networking enabled and disabled; the
save_state oracle passes, including the new lane, and fails when the
wait is neutered.

Not verified on a device: whether the message reads well in the case
that matters, which is the slow-storage close it exists for.

* runloop: introduce the content-closing state, observed by nothing

Stage 2 of unblocking content close.  A no-op by construction: the
state is set and cleared around the teardown that already exists, so
while it is true the main thread is inside that teardown and no
frame runs to ask about it.

It is introduced on its own precisely because the work that makes it
observable - returning to the frame loop instead of blocking on the
save wait - is a lifecycle change, and this is the piece the rest
will key off.  Reviewing the two separately means a problem with the
lifecycle change is one revert, not an unpicking.

A plain bool in runloop_state_t rather than a RUNLOOP_FLAG bit.
Bits 0-30 of that word are taken and bit 31 was deliberately vacated
to avoid a cross-thread race with the AppIntents entity query;
reusing it would undo that reasoning for no gain, and this is
main-thread only.

The clear is placed at the block's single exit, which was checked
rather than assumed: there is no early return or goto between the
set and the break, so one clear covers every path.  A leaked true
would be harmless today and would wedge the frame loop at stage 3,
which is exactly the kind of thing worth getting right while it
still cannot bite.

Verified: nothing reads the state except its own accessor, which
nothing calls - grepped rather than asserted, since "no-op" is the
entire claim here; builds with networking enabled and disabled; the
save_state oracle passes and the wait_timeout CI step was replayed
verbatim from the workflow.

* task_queue: fix two data races TSan found in CI

Two separate races, one mine and one older.

Mine, in the oracle: work_pending was cleared by the task HANDLER,
which runs on the worker thread, and read by the waiting main
thread.  A plain bool across two threads - a race on inspection,
with no tooling needed to see it.  It was also a misrepresentation
of the thing under test: the code being modelled,
netplay_mitm_query_cb(), clears its flag in the CALLBACK, which runs
on the thread pumping the queue.  Clearing it there fixes the race
and makes the test match what it claims to model.

The older one is in the queue itself.  retro_task_threaded_reset()
set RETRO_TASK_FLG_CANCELLED under running_lock, while the worker
reads task->flags under property_lock - two locks for one field.
It needed a reset to land while a worker was mid-task, which is why
it sat undisturbed until a lane that cancels an unfinished task
turned up.  The flags are now set under property_lock as well.
Nesting is safe: property_lock is never held while any other lock is
taken anywhere in this file, checked by walking every lock/unlock
pair, so it can only be an inner lock and no cycle exists.

Both are load-bearing, measured rather than assumed: 40 TSan runs
report 0 races with both fixes, and reverting the queue fix alone
brings it back at 6/40.

On the reporting of this: the commit that introduced the oracle said
it passed twice under TSan.  That was true - I ran it, three times
including the CI replay, and it passed each time.  The race
reproduces about once in twelve runs here, so three clean runs were
about a 78% chance of missing it.  The claim was accurate and the
verification was not adequate, which is the more useful way to say
it.  Repeat counts for anything threaded are now sized against how
rare the failure can be, not against how many runs feel like
enough - and a shared mutable flag written from a handler should
have been caught by reading it, which is where this should have
stopped.

Verified: 40 TSan runs clean on the changed lane; 8 each on the
other threaded oracles, since task_queue.c is shared by all of them;
every task oracle passes on a plain build; desktop build clean; the
CI step replayed verbatim from the workflow.

* runloop: don't run the core while content is closing

Stage 3 of unblocking content close.  Still inert, and deliberately
so: it establishes what the frame loop must do while the closing
state is set, before anything can set it with frames running.

retro_run() must not be entered once closing has begun, because the
teardown unloads the library that function lives in.  The guard
therefore has to exist before the wait moves out of the close path,
not with it - otherwise the change that lets frames run would be
introducing both the opportunity and the protection at once.

That it cannot fire today was checked rather than assumed: both
core_run() call sites are inside runloop_iterate(), and the content
close block in CMD_EVENT_CORE_DEINIT reaches neither - it calls no
iterate, no core_run and no run_ahead, so the main thread cannot be
in the frame loop while the flag is set.

It polls input and presents a cached frame rather than returning
bare, matching the netplay-paused case immediately below, and for
the same reason: a frame that simply stops being produced reads as a
hang, which is the whole complaint being worked on here.

Verified: builds with networking enabled and disabled; the
save_state oracle passes, including the lane that pins the close
wait; the wait_timeout CI step replayed verbatim from the workflow.

Worth stating before stage 4: at that point this guard stops being
inert, and video_driver_cached_frame() will be running while a core
is being torn down.  That is the first thing to look at if the frame
loop misbehaves during a close, and it is why the harness lane for
content close belongs before that change rather than after it.

* runloop: fix C89 violation in core_run

The content-closing guard was placed before netplay_preframe's
declaration, so that declaration followed a statement:

  runloop.c:8808: error: ISO C90 forbids mixed declarations and code

netplay_preframe is now declared with the other locals and assigned
where it was, so the guard still returns before netplay's pre-frame
call and the declarations stay at the top of the block.

My own standing rule is that every C file gets the strict C89 gate
before commit, and I did not run it on this one - the ordinary
desktop build does not use those flags, so it compiled clean and I
took that as sufficient.  It is not, and the gate exists precisely
because it is not.

Verified with the flags the C89 build actually uses, on runloop.c
with networking both enabled and disabled, and with a full
C89_BUILD=1 compile of the tree; reinstating the original ordering
reproduces the reported error, so the check is real rather than
vacuous.

* samples: drive the content-closing guard against the real frame loop

Stage 4 will stop blocking on the save wait and let frames run while
a close finishes.  The guard that makes that safe - core_run()
refusing to enter retro_run() while content_closing is set - is
already in place, and is at present completely unexercised: closing
is synchronous, so no frame runs during it, and the guard would go
live in the same change that first creates the situation it protects
against.  If it were wrong, the symptom would be a call into an
unloaded dylib.

So it is driven here first, directly: the real core_run(), in a real
frontend brought up headless, with the flag set.  Links the shipping
objects with only main() replaced; nothing is stubbed.

Three assertions, and the first exists to stop the others passing
vacuously: with the flag CLEAR the core must actually be entered, or
this harness would be proving nothing about a core_run() that never
reaches retro_run() in this environment anyway.  Then with the flag
set it must not be entered, and once cleared it must resume - a
sticky guard would leave the frame loop dead after a close.  A
second lane runs 240 frames with the flag set, the shape of a close
on slow storage, checking core_run() returns cleanly every time
rather than faulting or wedging.

Removing the guard fails both lanes with exactly the message they
exist to produce.

It joins the existing menu-harness workflow, which already makes the
non-Qt build these harnesses need - with Qt enabled ui_qt.o defines
main and the link collides - so it adds about two seconds rather
than another full build.  The step was replayed verbatim from the
workflow before commit, and the menu harness alongside it still
passes.

Verified: strict C89 gate on runloop.c with networking on and off,
which is the check I skipped last time and should not have.

* retroarch: extract the content-close teardown into its own function

Preparation for removing the close freeze, and nothing else: the
sixty lines that run after the save/load wait move into
command_event_finish_content_deinit(), still called from exactly
where they used to run.

They have to become resumable.  The wait above them is the freeze
this work is about, and removing it means this half runs later -
from the frame loop, once the tasks have finished - so it needs to
be callable from somewhere other than the middle of a switch case.
Doing the extraction on its own makes the next change a question of
WHEN it is called rather than what it does, which is the difference
between a revert and an unpicking if that goes wrong.

Every local is re-derived from its accessor rather than passed in,
so the function carries no dependency on the caller's frame - which
is what lets it be called from the frame loop later.

The extracted body was compared statement by statement against the
original block: 53 statements, zero differences.  That check was
worth writing, because the first attempt at this anchored on a
comment that appears twice in the file and silently swallowed an
unrelated case - it failed to compile, but a subtler overshoot would
not have.

Verified: strict C89 gate on retroarch.c with networking on and off;
builds with Qt and headless; both full-program harnesses pass, and
the save_state oracle with them.

---------

Co-authored-by: LibretroAdmin <[email protected]>
This commit is contained in:
LibretroAdminandLibretroAdmin authored and GitHub committed 2026-08-16 22:11:06 +02:00
1 parent 9f998727ca
commit 334e8b7baf
17 files changed
+1128 -80
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

.github/workflows/Linux-menu-harness.yml
+19
View File
@@ -70,6 +70,25 @@ jobs:
--disable-ffmpeg
make -j$(nproc)
- name: Build and run content_closing_test
shell: bash
run: |
set -eu
# The guard that keeps the frame loop out of a core being
# torn down (core_run() refusing retro_run() while
# content_closing is set) cannot fire in shipped code yet:
# closing is still synchronous, so no frame runs during it.
# It goes live in the change that first lets frames run
# during a close, and if it is wrong the symptom is a call
# into an unloaded dylib - so it is driven here directly,
# against the real core_run() in a real frontend, rather
# than going live untested.
#
# Shares the non-Qt build made above; adds about a second.
samples/runloop/content_closing/build.sh
./samples/runloop/content_closing/content_closing_test
make -C samples/runloop/content_closing clean
- name: Build and run menu_playlist_nav_test
shell: bash
run: |
.github/workflows/Linux-samples-tasks.yml
+30
View File
@@ -616,6 +616,36 @@ jobs:
TSAN_OPTIONS=halt_on_error=1 ./content_prefetch_test
make clean >/dev/null
- name: Build and run wait_timeout_test (plain, ASan + UBSan, TSan)
shell: bash
working-directory: samples/tasks/wait_timeout
run: |
set -eu
# task_queue_wait() waits indefinitely, which is wrong for
# anything depending on a machine we do not control - the
# netplay MITM query waits on an HTTP round trip to the
# lobby server, and a server that never answers used to hang
# the frontend outright.
#
# These lanes pin both directions: a wait that CAN be
# satisfied returns as soon as it is, rather than sitting
# out its timeout, and one that cannot gives up at roughly
# the deadline instead of never. Both queue implementations
# are covered, since the bound is a wrapped condition rather
# than per-implementation code.
make clean >/dev/null
make all -j$(nproc)
./wait_timeout_test
make clean >/dev/null
make all SANITIZER=address,undefined -j$(nproc)
ASAN_OPTIONS=detect_leaks=1:detect_stack_use_after_return=1 \
UBSAN_OPTIONS=print_stacktrace=1:halt_on_error=1 \
./wait_timeout_test
make clean >/dev/null
make all SANITIZER=thread -j$(nproc)
TSAN_OPTIONS=halt_on_error=1 ./wait_timeout_test
make clean >/dev/null
- name: Build and run task_ordering_test (plain, ASan + UBSan, TSan)
shell: bash
working-directory: samples/tasks/ordering
content.h
+4
View File
@@ -75,6 +75,10 @@ bool content_serialize_state_rewind(void* buffer, size_t buffer_size);
/* Deserializes the current state. */
bool content_deserialize_state(const void* serialized_data, size_t serialized_size);
/* True while a save state task is in progress, i.e. while
* content_wait_for_save_state_task() would block. */
bool content_save_state_in_progress(void* data);
/* Waits for any in-progress save state tasks to finish */
void content_wait_for_save_state_task(void);
/* Waits for any in-progress load state tasks to finish */
libretro-common/include/queues/task_queue.h
+20
View File
@@ -669,6 +669,26 @@ bool task_queue_push(retro_task_t *task);
*/
void task_queue_wait(retro_task_condition_fn_t cond, void* data);
/**
* task_queue_wait_timeout:
* @cond : Condition to keep waiting on; waiting ends when
* it returns false, as with task_queue_wait().
* @data : Userdata passed to @cond.
* @timeout_usec : Upper bound on the wait, in microseconds.
*
* As task_queue_wait(), but gives up after @timeout_usec rather than
* waiting indefinitely. For waits that depend on something outside
* the machine - a network round trip, say - where "never" is a
* reachable outcome and hanging the frontend is not an acceptable
* response to it.
*
* @return true when @cond is no longer satisfied, i.e. the thing
* being waited for finished; false when the timeout was reached with
* @cond still true, so the caller must handle not having it.
*/
bool task_queue_wait_timeout(retro_task_condition_fn_t cond, void *data,
retro_time_t timeout_usec);
/**
* Marks all tasks in the queue as cancelled.
*
libretro-common/queues/task_queue.c
+53
View File
@@ -592,8 +592,19 @@ static void retro_task_threaded_reset(void)
retro_task_t *task = NULL;
slock_lock(running_lock);
/* task->flags is guarded by property_lock, not running_lock: the
* worker reads it there to decide whether a task has finished.
* Setting it under running_lock alone left the two using
* different locks for the same field, which is a data race - one
* TSan reports when a reset lands while the worker is mid-task.
*
* Nesting is safe here: property_lock is never held while any
* other lock is taken anywhere in this file, so it can only ever
* be an inner lock and no cycle is possible. */
slock_lock(property_lock);
for (task = tasks_running.front; task; task = task->next)
task->flags |= RETRO_TASK_FLG_CANCELLED;
slock_unlock(property_lock);
slock_unlock(running_lock);
}
@@ -1090,6 +1101,48 @@ void task_queue_wait(retro_task_condition_fn_t cond, void* data)
impl_current->wait(cond, data);
}
struct task_wait_deadline
{
retro_task_condition_fn_t cond;
void *data;
retro_time_t deadline;
};
/* Wraps the caller's condition so that it also goes false once the
* deadline passes. Doing it this way rather than adding a wait
* variant to the implementation vtable means every implementation -
* regular, threaded and GCD - keeps its own waiting behaviour, and
* no platform-specific code has to be touched to gain a bound. */
static bool task_queue_deadline_cond(void *data)
{
struct task_wait_deadline *d = (struct task_wait_deadline*)data;
if (cpu_features_get_time_usec() >= d->deadline)
return false;
return d->cond ? d->cond(d->data) : true;
}
bool task_queue_wait_timeout(retro_task_condition_fn_t cond, void *data,
retro_time_t timeout_usec)
{
struct task_wait_deadline d;
d.cond = cond;
d.data = data;
d.deadline = cpu_features_get_time_usec() + timeout_usec;
task_queue_wait(task_queue_deadline_cond, &d);
/* The wait also ends when the queue drains, so "did the deadline
* pass" is not the same question as "is the caller still waiting
* for something". Ask the caller's own condition. */
if (cond && cond(data))
return false;
return true;
}
void task_queue_reset(void)
{
impl_current->reset();
network/netplay/netplay.h
+13
View File
@@ -210,6 +210,19 @@ void deinit_netplay(void);
bool netplay_driver_ctl(enum rarch_netplay_ctl_state state, void *data);
/**
* netplay_mitm_query_prefetch:
*
* Start the relay tunnel query early, when the user commits to
* hosting, rather than when host setup needs the address. The round
* trip then overlaps work that was going to happen anyway, so the
* wait for it usually costs nothing.
*
* Best effort: if it is skipped or fails, host setup issues the
* query exactly as it did before.
*/
void netplay_mitm_query_prefetch(void);
bool netplay_reinit_serialization(void);
bool netplay_is_spectating(void);
void netplay_force_send_savestate(void);
network/netplay/netplay_frontend.c
+172 -20
View File
@@ -21,6 +21,7 @@
#include <stdlib.h>
#include <stdio.h>
#include <stdint.h> /* uintptr_t, for the query generation token */
#include <sys/types.h>
#include <file/archive_file.h>
#include <streams/interface_stream.h>
@@ -9015,6 +9016,27 @@ static void netplay_announce(netplay_t *netplay)
* not, so there is no completion that leaves this set. */
static bool netplay_mitm_query_pending = false;
/* Incremented for every query issued, and again when one is
* abandoned. The callback carries the value it was issued with and
* does nothing if it no longer matches.
*
* Without this a timed-out query is still in flight, and its
* callback - which writes host_room->mitm_address and mitm_port
* unconditionally - would land after the caller had already fallen
* back to direct mode, publishing a tunnel address for a session
* that is not tunnelled. Bounding the wait without this guard would
* trade a hang for silent corruption. */
static unsigned netplay_mitm_query_generation = 0;
/* The handle the outstanding query was issued for.
*
* A query can now be started before host setup runs, so by the time
* netplay_mitm_query() is reached the answer may already be in hand
* or on its way. This is what lets it tell "already asked for this"
* from "asked for something else", so a user who changes the relay
* setting between the two points still gets the server they picked. */
static char netplay_mitm_query_handle[NAME_MAX_LENGTH] = {0};
static bool netplay_mitm_query_is_pending(void *data)
{
return netplay_mitm_query_pending;
@@ -9029,6 +9051,9 @@ static void netplay_mitm_query_cb(retro_task_t *task, void *task_data,
net_driver_state_t *net_st = &networking_driver_st;
struct netplay_room *host_room = &net_st->host_room;
if ((unsigned)(uintptr_t)user_data != netplay_mitm_query_generation)
return; /* superseded or abandoned; not ours to answer */
netplay_mitm_query_pending = false;
if (err || !data || !data->data || !data->len || data->status != 200)
@@ -9077,12 +9102,35 @@ static void netplay_mitm_query_cb(retro_task_t *task, void *task_data,
free(buf);
}
static bool netplay_mitm_query(const char *handle)
/* Upper bound on the wait for tunnel information from the lobby
* server, in microseconds. Generous on purpose: timing out a query
* that would have succeeded costs the user their hosted session,
* while waiting a few seconds longer costs only patience. */
#define NETPLAY_MITM_QUERY_TIMEOUT (10 * 1000 * 1000)
/* The tunnel query, split into begin / is_ready / result.
*
* Split rather than moved: netplay_mitm_query() below still calls all
* three back to back, so behaviour is exactly what it was. The point
* is that "ask" and "wait for the answer" are no longer welded
* together, which is what a later change needs in order to start the
* query when the user opens the host UI and have the answer waiting
* by the time host setup runs.
*
* Returns false if the query could not be issued at all. A custom
* relay needs no query, so it fills the address in directly and
* reports ready immediately. */
static bool netplay_mitm_query_begin(const char *handle)
{
net_driver_state_t *net_st = &networking_driver_st;
struct netplay_room *host_room = &net_st->host_room;
if (!handle || !*handle)
return false;
strlcpy(netplay_mitm_query_handle, handle,
sizeof(netplay_mitm_query_handle));
/* We don't need to query,
if we are using a custom relay server. */
if (string_is_equal(handle, "custom"))
@@ -9101,43 +9149,147 @@ static bool netplay_mitm_query(const char *handle)
strlcpy(host_room->mitm_address, addr, sizeof(host_room->mitm_address));
host_room->mitm_port = (int)port;
/* Supersede anything still in flight. A custom relay answers
* synchronously, and the old code returned here without ever
* consulting the pending flag - so an earlier query left
* outstanding must not make this one wait, and its callback
* must not later overwrite the address just set. */
++netplay_mitm_query_generation;
netplay_mitm_query_pending = false;
return true;
}
else
{
char query[256];
size_t _len = strlcpy(query, FILE_PATH_LOBBY_LIBRETRO_URL "tunnel?name=",
sizeof(query));
strlcpy(query + _len, handle, sizeof(query) - _len);
netplay_mitm_query_pending = true;
++netplay_mitm_query_generation;
if (!task_push_http_transfer(query, true, NULL,
netplay_mitm_query_cb, NULL))
netplay_mitm_query_cb,
(void*)(uintptr_t)netplay_mitm_query_generation))
{
netplay_mitm_query_pending = false;
return false;
}
/* Make sure we've the tunnel address before continuing.
*
* Host setup below needs the tunnel address, so this still
* blocks - but only on this one query. It used to wait on a
* NULL condition, which means "until the task queue is empty":
* every unrelated task in flight had to finish first, so
* starting a hosted session behind an in-progress core
* download or content scan blocked the UI for the whole of
* that download, not for an HTTP round trip.
*
* Removing the block entirely means deferring the rest of
* netplay host initialisation into this query's callback -
* sockets, announcement and lobby registration all read the
* address decided here - which is a netplay restructuring, not
* a change to this wait. */
task_queue_wait(netplay_mitm_query_is_pending, NULL);
}
return true;
}
/* Whether the answer has arrived. A custom relay was answered
* synchronously in begin(), so this is true straight away there. */
static bool netplay_mitm_query_ready(void)
{
return !netplay_mitm_query_pending;
}
/* Blocks until the answer arrives or the bound expires.
*
* The bound matters because this is a round trip to a machine we do
* not control. A lobby server that is down, blackholed or simply
* slow left the frontend hung with no way out but killing it. On
* timeout this reports failure like any other query and the caller
* falls back to direct mode with a warning - an outcome the user can
* see and act on, which an indefinite hang is not.
*
* Generous on purpose: a false timeout breaks hosting that would have
* worked, which is worse than the wait it prevents. */
static bool netplay_mitm_query_await(void)
{
if (netplay_mitm_query_ready())
return true;
if (!task_queue_wait_timeout(netplay_mitm_query_is_pending, NULL,
NETPLAY_MITM_QUERY_TIMEOUT))
{
RARCH_WARN("[Netplay] Timed out waiting for tunnel information"
" from the lobby server.\n");
/* Bump the generation so the in-flight callback, which may
* still arrive, does not write an address for a session that
* has already fallen back to direct mode. */
++netplay_mitm_query_generation;
netplay_mitm_query_pending = false;
return false;
}
return true;
}
/* What the query produced: an address and port, or nothing. */
static bool netplay_mitm_query_result(void)
{
net_driver_state_t *net_st = &networking_driver_st;
struct netplay_room *host_room = &net_st->host_room;
return *host_room->mitm_address && host_room->mitm_port;
}
/* Whether an answer for @handle is already in hand or on its way,
* so asking again would be a second round trip for the same thing.
*
* A prefetched query that came back EMPTY is deliberately not
* reused: a transient lobby failure at prefetch time would otherwise
* doom a hosted session that asking again would have got, which is a
* worse trade than one extra request. */
static bool netplay_mitm_query_have(const char *handle)
{
if (!handle || !*handle)
return false;
if (!string_is_equal(handle, netplay_mitm_query_handle))
return false;
return !netplay_mitm_query_ready() || netplay_mitm_query_result();
}
static bool netplay_mitm_query(const char *handle)
{
if (!netplay_mitm_query_have(handle))
if (!netplay_mitm_query_begin(handle))
return false;
if (!netplay_mitm_query_await())
return false;
return netplay_mitm_query_result();
}
/* Start the tunnel query early.
*
* Called when the user commits to hosting, which is well before host
* setup needs the address - and on the content-reload path, before
* the content has even loaded. The query runs during that time, so
* by the time netplay_mitm_query() is reached the answer is usually
* already there and its wait costs nothing.
*
* Best effort in the strictest sense: it changes nothing except when
* the round trip happens. If it fails, is skipped, or the user
* changes relay servers afterwards, host setup issues the query
* exactly as it did before. */
void netplay_mitm_query_prefetch(void)
{
settings_t *settings = config_get_ptr();
const char *handle;
if (!settings || !settings->bools.netplay_use_mitm_server)
return;
handle = settings->arrays.netplay_mitm_server;
if (!handle || !*handle)
return;
/* Already asked, and the answer is here or coming. */
if (netplay_mitm_query_have(handle))
return;
netplay_mitm_query_begin(handle);
}
int16_t input_state_net(unsigned port, unsigned device,
unsigned idx, unsigned id)
{
retroarch.c
+134 -58
View File
@@ -3213,6 +3213,90 @@ bool is_accessibility_enabled(bool accessibility_enable, bool accessibility_enab
*
* Returns: true (1) on success, otherwise false (0).
**/
/* Everything closing content does once nothing is left running in
* the core.
*
* Split out of CMD_EVENT_CORE_DEINIT unchanged, and still called
* from exactly where it used to run. It is separated because it has
* to become resumable: the wait above it is the freeze this work is
* about, and removing that wait means this half runs later, from the
* frame loop, once the tasks have finished. Extracting it on its
* own keeps that change a question of WHEN this is called rather
* than what it does.
*
* Every local is re-derived from its accessor rather than passed in,
* so it carries no dependency on the caller's frame. */
static void command_event_finish_content_deinit(void)
{
runloop_state_t *runloop_st = runloop_state_get_ptr();
settings_t *settings = config_get_ptr();
video_driver_state_t
*video_st = video_state_get_ptr();
rarch_system_info_t *sys_info = &runloop_st->system;
struct retro_hw_render_callback *hwr = NULL;
/* Save last selected disk index, if required */
if (sys_info)
disk_control_save_image_index(&sys_info->disk_control);
runloop_runtime_log_deinit(runloop_st,
settings->bools.content_runtime_log,
settings->bools.content_runtime_log_aggregate,
settings->paths.directory_runtime_log,
settings->paths.directory_playlist);
content_reset_savestate_backups();
hwr = VIDEO_DRIVER_GET_HW_CONTEXT_INTERNAL(video_st);
#ifdef HAVE_CHEEVOS
rcheevos_unload();
#endif
#ifdef HAVE_NETWORKING
/* The core may have registered a netpacket interface
* (RETRO_ENVIRONMENT_SET_NETPACKET_INTERFACE). We hold a
* heap copy of that struct, but it carries function
* pointers into the core. Clear it before the core's dylib
* is closed by runloop_event_deinit_core(), otherwise those
* pointers dangle into unloaded code. Passing NULL frees and
* nulls the cached interface via the existing handler. */
netplay_driver_ctl(RARCH_NETPLAY_CTL_SET_CORE_PACKET_INTERFACE,
NULL);
#endif
runloop_event_deinit_core();
/* Clear turbo and hold button state on core unload */
{
input_driver_state_t *input_st = input_state_get_ptr();
if (input_st)
{
memset(&input_st->turbo_btns, 0, sizeof(turbo_buttons_t));
memset(&input_st->hold_btns, 0, sizeof(hold_buttons_t));
}
}
#ifdef HAVE_RUNAHEAD
/* If 'runahead_available' is false, then
* runahead is enabled by the user but an
* error occurred while the core was running
* (typically a save state issue). In this
* case we have to 'manually' reset the runahead
* runtime variables, otherwise runahead will
* remain disabled until the user restarts
* RetroArch */
if (runloop_st)
{
if (!(runloop_st->flags & RUNLOOP_FLAG_RUNAHEAD_AVAILABLE))
runahead_clear_variables(runloop_st);
/* Deallocate preemptive frames */
preempt_deinit(runloop_st);
}
#endif
if (hwr)
memset(hwr, 0, sizeof(*hwr));
}
bool command_event(enum event_command cmd, void *data)
{
struct rarch_state *p_rarch = &rarch_st;
@@ -4619,68 +4703,47 @@ bool command_event(enum event_command cmd, void *data)
* not - a manually triggered save (menu / hotkey /
* netplay) already in flight when the user closed
* content with savestate_auto_save disabled. */
/* Say what the pause is for before blocking on it.
*
* This does not shorten the wait - the invariant above
* means it cannot be skipped - but an unexplained frozen
* frame and a frame that says "saving state" are very
* different experiences, and on slow storage this is
* seconds. The message is pushed and one frame forced
* out first, because nothing draws once the wait starts.
*
* Only when there is actually something to wait for:
* content_save_state_in_progress() is the same condition
* the wait uses, so a close with no save in flight - the
* common case - is untouched and shows nothing. */
/* Closing content starts here.
*
* Set around the whole teardown, including the waits
* below: they are part of closing, and once the wait
* stops blocking they are the part that will still be
* running when the frame loop resumes. Nothing observes
* this yet - the main thread does not leave this block -
* which is the point of introducing it on its own. */
runloop_st->content_closing = true;
if (content_save_state_in_progress(NULL))
{
const char *_msg = msg_hash_to_str(MSG_SAVING_STATE);
runloop_msg_queue_push(_msg, strlen(_msg), 1, 180, true,
NULL, MESSAGE_QUEUE_ICON_DEFAULT,
MESSAGE_QUEUE_CATEGORY_INFO);
video_driver_cached_frame();
}
content_wait_for_save_state_task();
content_wait_for_load_state_task();
/* Save last selected disk index, if required */
if (sys_info)
disk_control_save_image_index(&sys_info->disk_control);
command_event_finish_content_deinit();
runloop_runtime_log_deinit(runloop_st,
settings->bools.content_runtime_log,
settings->bools.content_runtime_log_aggregate,
settings->paths.directory_runtime_log,
settings->paths.directory_playlist);
content_reset_savestate_backups();
hwr = VIDEO_DRIVER_GET_HW_CONTEXT_INTERNAL(video_st);
#ifdef HAVE_CHEEVOS
rcheevos_unload();
#endif
#ifdef HAVE_NETWORKING
/* The core may have registered a netpacket interface
* (RETRO_ENVIRONMENT_SET_NETPACKET_INTERFACE). We hold a
* heap copy of that struct, but it carries function
* pointers into the core. Clear it before the core's dylib
* is closed by runloop_event_deinit_core(), otherwise those
* pointers dangle into unloaded code. Passing NULL frees and
* nulls the cached interface via the existing handler. */
netplay_driver_ctl(RARCH_NETPLAY_CTL_SET_CORE_PACKET_INTERFACE,
NULL);
#endif
runloop_event_deinit_core();
/* Clear turbo and hold button state on core unload */
{
input_driver_state_t *input_st = input_state_get_ptr();
if (input_st)
{
memset(&input_st->turbo_btns, 0, sizeof(turbo_buttons_t));
memset(&input_st->hold_btns, 0, sizeof(hold_buttons_t));
}
}
#ifdef HAVE_RUNAHEAD
/* If 'runahead_available' is false, then
* runahead is enabled by the user but an
* error occurred while the core was running
* (typically a save state issue). In this
* case we have to 'manually' reset the runahead
* runtime variables, otherwise runahead will
* remain disabled until the user restarts
* RetroArch */
if (runloop_st)
{
if (!(runloop_st->flags & RUNLOOP_FLAG_RUNAHEAD_AVAILABLE))
runahead_clear_variables(runloop_st);
/* Deallocate preemptive frames */
preempt_deinit(runloop_st);
}
#endif
if (hwr)
memset(hwr, 0, sizeof(*hwr));
/* Closing content is finished. One clear covers the
* whole block: it has a single exit, with no early
* return or goto between the set above and here. */
runloop_st->content_closing = false;
break;
}
@@ -5396,6 +5459,19 @@ bool command_event(enum event_command cmd, void *data)
break;
case CMD_EVENT_NETPLAY_ENABLE_HOST:
{
/* Ask the lobby server for the tunnel address now.
*
* This is the point where the user has committed to
* hosting; host setup does not need the address until
* later, and on the content-reload path below not until
* content has finished loading. Starting the round trip
* here lets it overlap that, so the wait for it in host
* setup usually finds the answer already present.
*
* Best effort - if this is skipped or fails, host setup
* asks exactly as it did before. */
netplay_mitm_query_prefetch();
if (netplay_driver_ctl(RARCH_NETPLAY_CTL_USE_CORE_PACKET_INTERFACE, NULL))
{
netplay_driver_ctl(RARCH_NETPLAY_CTL_ENABLE_SERVER, NULL);
runloop.c
+36 -1
View File
@@ -414,6 +414,11 @@ runloop_state_t *runloop_state_get_ptr(void)
return &runloop_state;
}
bool runloop_is_content_closing(void)
{
return runloop_state.content_closing;
}
bool state_manager_frame_is_reversed(void)
{
#ifdef HAVE_REWIND
@@ -8776,7 +8781,37 @@ void core_run(void)
bool early_polling = (new_poll_type == POLL_TYPE_EARLY);
bool late_polling = (new_poll_type == POLL_TYPE_LATE);
#ifdef HAVE_NETWORKING
bool netplay_preframe = netplay_driver_ctl(
/* Declared with the other locals and assigned below, so the
* closing guard can return before netplay's pre-frame call
* without putting a declaration after a statement. */
bool netplay_preframe;
#endif
/* The core is being torn down: do not run it.
*
* retro_run() must not be entered once closing has begun, because
* the teardown unloads the library that function lives in.
*
* Today this cannot be reached - closing is synchronous, so the
* main thread sits inside the teardown and no frame runs - and
* the guard is placed first, inert, so that the change which does
* let frames run during a close is only about where the waiting
* happens, not about what the frame loop may touch.
*
* Poll and present anyway rather than returning bare, so input
* keeps being drained and whatever the close has put on screen
* keeps being drawn. Same shape as the netplay-paused case
* below, for the same reason: a frame that stops being produced
* reads as a hang. */
if (runloop_st->content_closing)
{
input_driver_poll();
video_driver_cached_frame();
return;
}
#ifdef HAVE_NETWORKING
netplay_preframe = netplay_driver_ctl(
RARCH_NETPLAY_CTL_PRE_FRAME, NULL);
if (!netplay_preframe)
runloop.h
+26
View File
@@ -323,6 +323,23 @@ struct runloop
bool perfcnt_enable;
bool paused_hotkey;
/* True from the moment closing content starts tearing the core
* down until the teardown is finished.
*
* Set and cleared around the existing synchronous teardown, so at
* present nothing can observe it as true: the main thread is
* inside that teardown for its whole duration and no frame runs.
* It is introduced separately, and deliberately inert, because
* the work that makes it observable - returning to the frame loop
* instead of blocking - is a lifecycle change, and this is the
* piece everything else will key off.
*
* A plain bool rather than a RUNLOOP_FLAG bit: bits 0-30 of that
* word are taken and bit 31 was deliberately vacated to avoid a
* cross-thread race, so reusing it would undo that reasoning for
* no gain. This is main-thread only. */
bool content_closing;
};
typedef struct runloop runloop_state_t;
@@ -489,6 +506,15 @@ bool runloop_init_libretro_symbols(
runloop_state_t *runloop_state_get_ptr(void);
/**
* runloop_is_content_closing:
*
* True while content is being closed, i.e. while the core is being
* torn down. Currently only ever true inside the synchronous
* teardown, where nothing else runs to ask.
*/
bool runloop_is_content_closing(void);
RETRO_END_DECLS
#endif
samples/runloop/content_closing/Makefile
+13
View File
@@ -0,0 +1,13 @@
# See build.sh: this harness links the shipping RetroArch objects with
# only main() replaced, so it needs the project's own build rather than
# a source list of its own.
TARGET := content_closing_test
all:
@./build.sh
clean:
rm -f $(TARGET) retroarch_nomain.o retroarch_nomain.d \
harness_main.o harness_main.d
.PHONY: all clean
samples/runloop/content_closing/build.sh
Executable
+72
View File
@@ -0,0 +1,72 @@
#!/bin/sh
# Builds the menu playlist navigation harness.
#
# Unlike every other sample, this does not compile a translation unit
# in isolation. It links the SHIPPING objects of a built RetroArch,
# with only main() replaced - nothing is stubbed. Three field reports
# came out of the seam between the playlist reader and the menu
# (a stale playlist's entries under a new heading; a blank list the
# back button could not leave; a list that filled only after the user
# pressed something), and no oracle that stopped at the reader could
# see any of them.
#
# Requires a completed NON-Qt build: with Qt enabled main() lives in
# ui_qt.o, which drags the whole Qt UI in with it. From the repo
# root:
#
# ./configure --disable-qt && make
# samples/runloop/content_closing/build.sh
#
# retroarch.c is recompiled here with main renamed, so the harness can
# supply its own without having to exclude the object that holds most
# of the frontend. Both the compile and the link reuse the project's
# own command lines, taken from make -n, so the harness cannot drift
# away from how the program is really built.
set -eu
here=$(cd "$(dirname "$0")" && pwd)
root=$(cd "$here/../../.." && pwd)
cd "$root"
if [ ! -f obj-unix/release/retroarch.o ]; then
echo "build RetroArch first: ./configure --disable-qt && make" >&2
exit 1
fi
cc_line=$(mktemp)
ld_line=$(mktemp)
trap 'rm -f "$cc_line" "$ld_line"' EXIT
# The project's own compile line for retroarch.o ...
touch retroarch.c
make -n 2>/dev/null | grep -E '\-o obj-unix/release/retroarch\.o' | head -1 > "$cc_line"
if [ ! -s "$cc_line" ]; then
echo "could not determine the compile command for retroarch.o" >&2
exit 1
fi
# ... reused twice: once for retroarch.c with main renamed out of the
# way, once for the harness itself, so both are built exactly the way
# the frontend is.
sed 's#-o obj-unix/release/retroarch\.o#-Dmain=rarch_harness_unused_main -o samples/runloop/content_closing/retroarch_nomain.o#' \
"$cc_line" | sh
sed -e 's#-o obj-unix/release/retroarch\.o#-o samples/runloop/content_closing/harness_main.o#' \
-e 's# retroarch\.c# samples/runloop/content_closing/content_closing_test.c#' \
"$cc_line" | sh
# The project's own link line, with retroarch.o swapped for the pair
# above. Removing the binary first is what makes make -n emit it.
rm -f retroarch
make -n 2>/dev/null | grep -E ' -o retroarch ' | tail -1 > "$ld_line"
if [ ! -s "$ld_line" ]; then
echo "could not determine the link command" >&2
exit 1
fi
sed -e 's#obj-unix/release/retroarch\.o#samples/runloop/content_closing/retroarch_nomain.o samples/runloop/content_closing/harness_main.o#' \
-e 's#-o retroarch #-o samples/runloop/content_closing/content_closing_test #' \
"$ld_line" | sh
echo "built samples/runloop/content_closing/content_closing_test"
samples/runloop/content_closing/content_closing_test.c
+209
View File
@@ -0,0 +1,209 @@
/* Harness for the content-closing state, driving the real frame loop.
*
* Closing content tears the core down, and the save and load handlers
* call into that core - retro_serialize via
* content_get_serialized_data, retro_unserialize via
* core_unserialize. A worker still inside one of those when
* runloop_event_deinit_core() unloads the library dispatches into
* freed code, which is why the close waits for those tasks and why
* that wait cannot simply be deleted.
*
* The plan for removing the freeze is to stop blocking and let the
* frame loop keep running while the close finishes. The guard that
* makes that safe - core_run() refusing to enter retro_run() while
* runloop_state.content_closing is set - is in place but cannot fire
* yet, because the close is still synchronous and no frame runs
* during it.
*
* Which means the guard is, right now, entirely unexercised: it will
* go live in the same change that first lets frames run during a
* close, and if it is wrong the symptom is a call into an unloaded
* dylib. This drives it directly instead - the real core_run(), in
* a real frontend, with the flag set - so that when it does go live
* it is not going live untested.
*
* Links the shipping objects with only main() replaced; nothing is
* stubbed.
*/
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <boolean.h>
#include "../../../runloop.h"
#include "../../../retroarch.h"
#include "../../../configuration.h"
#include "../../../frontend/frontend_driver.h"
#include "../../../verbosity.h"
#include <time/rtime.h>
#include <file/config_file.h>
static unsigned failures = 0;
#define CHECK(cond, ...) \
do { \
if (!(cond)) \
{ \
fprintf(stderr, "FAIL %s:%d: ", __FILE__, __LINE__); \
fprintf(stderr, __VA_ARGS__); \
fprintf(stderr, "\n"); \
failures++; \
} \
} while (0)
/* ------------------------------------------------------------------ */
/* A core whose retro_run only counts */
/* ------------------------------------------------------------------ */
static unsigned retro_run_calls;
static void counting_retro_run(void)
{
retro_run_calls++;
}
/* ------------------------------------------------------------------ */
static void lane_closing_blocks_retro_run(void)
{
unsigned had = failures;
runloop_state_t *runloop_st = runloop_state_get_ptr();
void (*saved_run)(void) = runloop_st->current_core.retro_run;
runloop_st->current_core.retro_run = counting_retro_run;
/* Not closing: the core runs, which is what makes the next
* assertion mean something rather than passing because core_run()
* never reaches retro_run() at all in this environment. */
runloop_st->content_closing = false;
retro_run_calls = 0;
core_run();
CHECK(retro_run_calls == 1,
"core_run() did not reach retro_run() with the flag clear, "
"so this harness cannot tell whether the guard works");
/* Closing: the core must not be entered. This is the property
* that keeps the frame loop off a core whose library is going
* away. */
runloop_st->content_closing = true;
retro_run_calls = 0;
core_run();
CHECK(retro_run_calls == 0,
"core_run() entered retro_run() while content was closing - "
"with the teardown unloading that library, this is a call "
"into freed code");
/* And back: the guard must not be sticky, or the frame loop would
* stay dead after a close finished. */
runloop_st->content_closing = false;
retro_run_calls = 0;
core_run();
CHECK(retro_run_calls == 1,
"the core did not resume once closing had finished");
runloop_st->current_core.retro_run = saved_run;
runloop_st->content_closing = false;
if (failures == had)
fprintf(stderr, "[pass] closing-blocks-retro-run lane\n");
}
/* Frames must keep being produced while closing, or the window stops
* updating and the close reads as a hang - the very complaint this
* work exists to fix. */
static void lane_closing_still_presents(void)
{
unsigned had = failures;
runloop_state_t *runloop_st = runloop_state_get_ptr();
void (*saved_run)(void) = runloop_st->current_core.retro_run;
unsigned i;
runloop_st->current_core.retro_run = counting_retro_run;
runloop_st->content_closing = true;
retro_run_calls = 0;
/* Many frames, as a close on slow storage would take. The point
* is that core_run() returns cleanly every time rather than
* faulting on a torn-down core or wedging. */
for (i = 0; i < 240; i++)
core_run();
CHECK(retro_run_calls == 0,
"the core was entered during a 240-frame close");
runloop_st->current_core.retro_run = saved_run;
runloop_st->content_closing = false;
if (failures == had)
fprintf(stderr, "[pass] closing-still-presents lane (240 frames)\n");
}
/* ------------------------------------------------------------------ */
int main(int argc, char *argv[])
{
char cfg_path[512];
char cmd[600];
char dir[400];
char *rarch_argv[8];
int rarch_argc = 0;
FILE *cfg;
snprintf(dir, sizeof(dir), "/tmp/closing_harness_%ld", (long)getpid());
snprintf(cmd, sizeof(cmd), "mkdir -p %s", dir);
if (system(cmd) != 0)
return 1;
snprintf(cfg_path, sizeof(cfg_path), "%s/harness.cfg", dir);
if ((cfg = fopen(cfg_path, "wb")))
{
fprintf(cfg, "video_driver = \"null\"\n");
fprintf(cfg, "audio_driver = \"null\"\n");
fprintf(cfg, "input_driver = \"null\"\n");
fprintf(cfg, "input_joypad_driver = \"null\"\n");
fprintf(cfg, "menu_driver = \"rgui\"\n");
fprintf(cfg, "video_threaded = \"false\"\n");
fclose(cfg);
}
/* The prelude rarch_main() performs before main_init; skipping any
* of it is a null dereference deep inside. */
config_file_set_io_default(config_file_io_filestream());
rtime_init();
retroarch_config_init();
retroarch_ctl(RARCH_CTL_STATE_FREE, NULL);
frontend_driver_init_first(NULL);
rarch_argv[rarch_argc++] = (char*)"retroarch";
rarch_argv[rarch_argc++] = (char*)"--menu";
rarch_argv[rarch_argc++] = (char*)"--config";
rarch_argv[rarch_argc++] = cfg_path;
if (!retroarch_main_init(rarch_argc, rarch_argv))
{
fprintf(stderr, "FAIL: retroarch_main_init failed\n");
return 1;
}
lane_closing_blocks_retro_run();
lane_closing_still_presents();
snprintf(cmd, sizeof(cmd), "rm -rf %s", dir);
if (system(cmd) != 0) { }
if (failures)
{
fprintf(stderr, "FAIL content_closing_test: %u failures\n",
failures);
return 1;
}
fprintf(stderr, "PASS content_closing_test\n");
return 0;
}
samples/tasks/save_state/save_state_io_test.c
+51
View File
@@ -988,6 +988,56 @@ static void test_undo_allocates_nothing(void)
content_reset_savestate_backups();
}
/* -----------------------------------------------------------------
* Closing content while a save is in flight.
*
* runloop_event_deinit_core() unloads the core's dylib, and the save
* and load handlers call into it - retro_serialize via
* content_get_serialized_data, retro_unserialize via
* core_unserialize. A worker still inside one of those when the
* library goes away dispatches into freed code. That is why closing
* content calls content_wait_for_save_state_task(), and why that
* wait cannot simply be deleted the way the other blocking waits in
* this series were.
*
* What this pins is the property the unload depends on: the wait
* does not return until the save has actually finished. If it ever
* returned early - on a budget boundary, say - the unload would land
* while a worker was still inside the core.
*
* The file is checked incomplete before the wait and complete after,
* so a wait that silently became a no-op fails here rather than
* turning into a crash on a device.
* ----------------------------------------------------------------- */
static void test_close_waits_for_save(void)
{
const char *path = "sst_close_wait.state";
size_t sz = 64 * TEST_SAVE_STATE_CHUNK; /* 6.25 MB */
long before;
frontend_reset();
core_fill(sz);
filestream_delete(path);
/* One quantum per tick: the slow-storage case, where a close is
* long enough for the user to feel it. */
clock_step = TEST_TICK_BUDGET_US;
content_save_state(path, true);
before = file_size(path);
okf(before != (long)content_get_serialized_size(),
"the save is genuinely unfinished when the close begins");
/* Exactly what closing content does. */
content_wait_for_save_state_task();
okf(file_size(path) == (long)content_get_serialized_size(),
"closing content waits for the save to finish before "
"the core could be unloaded");
filestream_delete(path);
}
static int run_default_lane(void)
{
printf("== task_save.c I/O regression oracle ==\n");
@@ -1001,6 +1051,7 @@ static int run_default_lane(void)
test_roundtrip(0, "round-trip is byte-exact with an unexpired budget");
test_roundtrip(TEST_TICK_BUDGET_US,
"round-trip is byte-exact at one quantum per tick");
test_close_waits_for_save();
test_serialize_failure();
test_open_failure();
test_truncated_state();
samples/tasks/wait_timeout/Makefile
+41
View File
@@ -0,0 +1,41 @@
TARGET := wait_timeout_test
# Drives the shipping libretro-common/queues/task_queue.c directly.
# The subject is task_queue_wait_timeout(): that a wait which can
# never be satisfied - a network round trip to a server that does not
# answer - gives up instead of hanging the frontend, and that a wait
# which IS satisfied returns immediately rather than sitting out its
# timeout.
REPO_ROOT := ../../..
LIBRETRO_COMM_DIR := $(REPO_ROOT)/libretro-common
SOURCES := wait_timeout_test.c \
$(LIBRETRO_COMM_DIR)/queues/task_queue.c \
$(LIBRETRO_COMM_DIR)/rthreads/rthreads.c \
$(LIBRETRO_COMM_DIR)/features/features_cpu.c \
$(LIBRETRO_COMM_DIR)/compat/compat_strl.c
OBJS := $(SOURCES:.c=.o)
CFLAGS += -Wall -std=gnu99 -g -DHAVE_THREADS \
-I$(LIBRETRO_COMM_DIR)/include
LDFLAGS += -lpthread -lm
# The samples workflow passes SANITIZER=address,undefined; honour it.
ifneq ($(SANITIZER),)
CFLAGS := -fsanitize=$(SANITIZER) -fno-omit-frame-pointer $(CFLAGS)
LDFLAGS := -fsanitize=$(SANITIZER) $(LDFLAGS)
endif
all: $(TARGET)
%.o: %.c
$(CC) -c -o $@ $< $(CFLAGS)
$(TARGET): $(OBJS)
$(CC) -o $@ $^ $(LDFLAGS)
clean:
rm -f $(TARGET) $(OBJS)
.PHONY: clean all
samples/tasks/wait_timeout/wait_timeout_test.c
+230
View File
@@ -0,0 +1,230 @@
/* Oracle for task_queue_wait_timeout().
*
* task_queue_wait() waits indefinitely, which is correct for work
* that is certain to finish and wrong for anything depending on
* something outside the machine. The netplay MITM query is the
* motivating case: it waits on an HTTP round trip to the lobby
* server, and a server that never answers hangs the frontend with no
* way out.
*
* The bound is implemented by wrapping the caller's condition in one
* that also goes false at a deadline, so each queue implementation -
* regular, threaded, GCD - keeps its own waiting behaviour and no
* platform-specific code is involved. What has to hold:
*
* completes - a wait whose work finishes returns as soon as it
* does, reports success, and does NOT sit out the
* remaining timeout.
* times out - a wait whose condition never clears returns anyway,
* reports failure, and takes about the timeout rather
* than forever.
* no-op - a condition already false returns immediately.
* threaded - the same holds on the threaded queue, where the
* waiting is a different implementation.
*
* The timing assertions are deliberately loose at the top end: this
* runs on shared CI machines and the point is "bounded, not
* indefinite", not a precise duration. The lower bounds are tight,
* because returning EARLY would silently break the caller.
*/
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <boolean.h>
#include <queues/task_queue.h>
#include <features/features_cpu.h>
#include <retro_timers.h>
static unsigned failures = 0;
#define CHECK(cond, ...) \
do { \
if (!(cond)) \
{ \
fprintf(stderr, "FAIL %s:%d: ", __FILE__, __LINE__); \
fprintf(stderr, __VA_ARGS__); \
fprintf(stderr, "\n"); \
failures++; \
} \
} while (0)
#define TIMEOUT_USEC 300000 /* 0.3s - long enough to measure */
/* ------------------------------------------------------------------ */
/* A task that finishes after N ticks, and one that never finishes */
/* ------------------------------------------------------------------ */
static int ticks_remaining;
static bool work_pending;
/* Handlers run on the WORKER thread under the threaded queue, so
* nothing here touches state the waiting thread reads. The flag is
* cleared in the callback below - which is where the code being
* modelled clears it, netplay_mitm_query_cb(), and callbacks run on
* the thread pumping the queue.
*
* Clearing it here was both a data race and a misrepresentation of
* the thing under test. */
static void finite_handler(retro_task_t *task)
{
if (ticks_remaining > 0)
{
ticks_remaining--;
return;
}
task_set_flags(task, RETRO_TASK_FLG_FINISHED, true);
}
static void endless_handler(retro_task_t *task)
{
/* Models a network wait: the task stays in the queue, and the
* thing the caller is waiting for never arrives. */
}
/* Runs on the thread pumping the queue - the waiting thread - just
* as the netplay query callback does. */
static void cb(retro_task_t *task, void *task_data, void *user_data,
const char *error)
{
work_pending = false;
}
/* The caller's own condition, as a netplay-style "is it still
* pending" flag rather than a queue lookup. */
static bool still_pending(void *data)
{
return work_pending;
}
static bool push(void (*handler)(retro_task_t*))
{
retro_task_t *task = task_init();
if (!task)
return false;
task->handler = handler;
task->callback = cb;
return task_queue_push(task);
}
/* ------------------------------------------------------------------ */
/* Lanes */
/* ------------------------------------------------------------------ */
static void lane_completes_before_timeout(bool threaded)
{
unsigned had = failures;
retro_time_t started, elapsed;
bool ok;
task_queue_init(threaded, NULL);
ticks_remaining = 3;
work_pending = true;
CHECK(push(finite_handler), "push failed");
started = cpu_features_get_time_usec();
ok = task_queue_wait_timeout(still_pending, NULL, TIMEOUT_USEC);
elapsed = cpu_features_get_time_usec() - started;
task_queue_deinit();
CHECK(ok, "%s: reported failure for work that finished",
threaded ? "threaded" : "regular");
CHECK(!work_pending, "%s: work did not actually finish",
threaded ? "threaded" : "regular");
/* The point of the bound is that it does not delay the ordinary
* case; sitting out the full timeout would be a regression that
* still "passes" a pass/fail check. */
CHECK(elapsed < (TIMEOUT_USEC / 2),
"%s: took %lldus for work that finished immediately - the "
"wait is sitting out its timeout instead of returning",
threaded ? "threaded" : "regular", (long long)elapsed);
if (failures == had)
fprintf(stderr, "[pass] completes-before-timeout (%s, %lldus)\n",
threaded ? "threaded" : "regular", (long long)elapsed);
}
static void lane_times_out(bool threaded)
{
unsigned had = failures;
retro_time_t started, elapsed;
bool ok;
task_queue_init(threaded, NULL);
work_pending = true;
CHECK(push(endless_handler), "push failed");
started = cpu_features_get_time_usec();
ok = task_queue_wait_timeout(still_pending, NULL, TIMEOUT_USEC);
elapsed = cpu_features_get_time_usec() - started;
/* Abandon the endless task so deinit is not left holding it. */
task_queue_reset();
task_queue_check();
task_queue_deinit();
CHECK(!ok, "%s: reported success though the work never finished",
threaded ? "threaded" : "regular");
/* Tight lower bound: giving up early would cut short a round trip
* that was about to succeed. */
CHECK(elapsed >= (TIMEOUT_USEC - (TIMEOUT_USEC / 10)),
"%s: gave up after %lldus, well before the %dus timeout",
threaded ? "threaded" : "regular", (long long)elapsed,
TIMEOUT_USEC);
/* Loose upper bound: the machine is shared, the claim is only
* that this terminates rather than hangs. */
CHECK(elapsed < (TIMEOUT_USEC * 10),
"%s: took %lldus against a %dus timeout - not bounded",
threaded ? "threaded" : "regular", (long long)elapsed,
TIMEOUT_USEC);
if (failures == had)
fprintf(stderr, "[pass] times-out (%s, %lldus)\n",
threaded ? "threaded" : "regular", (long long)elapsed);
}
static void lane_already_done(void)
{
unsigned had = failures;
retro_time_t started, elapsed;
bool ok;
task_queue_init(false, NULL);
work_pending = false;
started = cpu_features_get_time_usec();
ok = task_queue_wait_timeout(still_pending, NULL, TIMEOUT_USEC);
elapsed = cpu_features_get_time_usec() - started;
task_queue_deinit();
CHECK(ok, "reported failure for a condition already false");
CHECK(elapsed < (TIMEOUT_USEC / 2),
"waited %lldus for something already finished",
(long long)elapsed);
if (failures == had)
fprintf(stderr, "[pass] already-done lane\n");
}
int main(void)
{
lane_completes_before_timeout(false);
lane_times_out(false);
lane_already_done();
lane_completes_before_timeout(true);
lane_times_out(true);
if (failures)
{
fprintf(stderr, "FAIL wait_timeout_test: %u failures\n", failures);
return 1;
}
fprintf(stderr, "PASS wait_timeout_test\n");
return 0;
}
tasks/task_save.c
+5 -1
View File
@@ -1879,7 +1879,11 @@ static bool task_save_state_finder(retro_task_t *task, void *user_data)
}
/* Returns true if a save state task is in progress */
static bool content_save_state_in_progress(void* data)
/* True while a save state task is in progress.
*
* Public because closing content needs to know whether the wait
* below is going to block before it blocks, so it can say so. */
bool content_save_state_in_progress(void* data)
{
task_finder_data_t find_data;