Skip to content

Draft: SrtSubprocessRunner: Factor out an object for the execution environment

Simon McVittie requested to merge wip/task88 into main

Needs more testing, and should maybe be deferred until a future release - but I think this is a net improvement. @denittis, what do you think?


  • SrtSubprocessRunner: Factor out an object for the execution environment

    This will let us bundle together more of the subprocess handling for running helpers. For now, it is just a container for the environment, helpers path and test flags.

    To make this easier, make various functions that take an environment block more const-correct.

    No functional change intended, except that in the unlikely event that environ is NULL, pv-runtime and a unit test now call _srt_recursive_list_content() with a non-NULL environment array.

  • graphics: Pass execution environment to _srt_check_library_presence()

    Previously we ignored the parameter and used default values here.

  • library: Pass SrtSubprocessRunner to _srt_inspect_library

  • architecture: Remove an unused variable

  • spawn: Move _srt_get_helper to _srt_subprocess_runner_get_helper

  • lib: Add and use _srt_subprocess_runner_spawn_sync()

    This encapsulates g_spawn_sync() with a useful set of flags and behaviours, and will let us move the responsibility for handling features like timeouts between get_helper() and spawn_sync(). To make that work, try harder to pass the same flags to both get_helper() and spawn_sync().

    For architecture, display, graphics-drivers and libdl, no behaviour change is intended.

    display and graphics-drivers should probably not be using KEEP_GAMEOVERLAYRENDERER (there's no real reason to do so) but for now, preserve existing behaviour.

    For library, we were previously passing G_SPAWN_SEARCH_PATH to g_spawn_sync(), but it would have had no practical effect, because we're using the absolute path to a helper executable. Drop that flag.

    For graphics, we were previously searching the PATH for everything except check-gl. Consistently search the PATH in all cases.

  • lib: Remove SRT_HELPER_FLAGS_TIME_OUT_SOONER

    Now that SrtSubprocessRunner knows both the SrtTestFlags and SrtHelperFlags, it can always set the timeout appropriately.

  • lib: Always filter out gameoverlayrenderer from subprocesses' environment

    The display and graphics-drivers checks historically didn't, but that seems more like accident than design.

  • glib-backports: Provide G_SOURCE_FUNC

  • lib: Avoid using timeout(1) to impose a timeout on subprocesses

    This is more difficult than you might think, because we don't know whether this library is going to be loaded by a process that is blocking SIGCHLD or handling SIGCHLD itself, so we'd prefer to use the equivalent of g_spawn_sync(), which ends up in waitpid(), instead of relying on g_child_watch_source_new() working correctly.

    The key trick here is that on any halfway modern kernel, we can use waitid() in a background thread with the WNOWAIT flag, which waits for the subprocess to exit, but does not reap it, allowing the main thread to wait for it again (this time without WNOWAIT). It's not clear how old this functionality is, so I've kept the timeout(1) fallback for now.

    This made pressure-vessel/bin/steam-runtime-system-info take 17% less time when running under FEX-Emu (on Raspberry Pi 4B), where launching new executables is more expensive than usual because the FEX interpreter also needs to be started. On a fast x86 the difference is unclear (within measurement error).

    We might be able to make this a little more efficient by trying to use threads from a thread pool and reusing that thread pool across the lifetime of the SrtSubprocessRunner, but for now it's creating a new thread every time, which seems to be good enough in practice.

    steamrt/tasks#88

Merge request reports