unnecessarily uses a loop to wait for steam-runtime-launcher-service

[smcv]

Describe the bug
steam-runtime-launcher-service has a built-in mechanism to wait for it to be ready before proceeding, but Protontricks doesn't use it.

To Reproduce
n/a, from source code inspection

Expected behavior
If you run steam-runtime-launcher-service (or equivalently pressure-vessel-wrap --launcher or SteamLinuxRuntime_soldier/run --launcher) with the --info-fd option, then read from that file descriptor until EOF, that's enough to guarantee that the launcher service has either started up or failed to start. This would remove the need to run dbus-send in a loop.

The easiest way to achieve this would be:

• in bwrap_launcher.sh, add --info-fd=1 to the parameters passed after -- (1 means standard output);
• when you run it from Python code, run it with stdout=subprocess.PIPE;
• to wait for it to be ready, read from the Popen object's stdout: _start_process(...).stdout.read()

If you don't need any of the output, you can just discard it.

--info-fd=1 is in fact the default if you don't ask the launcher to wrap another command (so you're probably already getting this information and just sending it to /dev/null), but explicit is better than implicit.

There is man-page-style documentation in https://gitlab.steamos.cloud/steamrt/steam-runtime-tools/-/blob/main/bin/launcher-service.md and https://gitlab.steamos.cloud/steamrt/steam-runtime-tools/-/blob/main/bin/launch-client.md.

System (please complete the following information):
n/a, from source code inspection

[smcv]

Thanks for looking at this! 58bb325 will probably work in practice (because the launcher service doesn't output very much text, so the pipe buffer will not be full), but the intended use of --info-fd is that you continue to read from it until you reach end-of-file. If you don't need any of the information that is printed on that fd (in this case it looks like you don't), it's best to read and discard it.

In Python, launcher_process.stdout.read() is a convenient way to keep reading until EOF.

If you stop reading after the first byte, the launcher service could get stuck (blocked in write()) if we extend it to produce more output on the --info-fd in a later version than it currently does.

The unit tests in https://gitlab.steamos.cloud/steamrt/steam-runtime-tools/-/blob/main/tests/pressure-vessel/launcher.py include some tests that use the --info-fd.

[Matoking]

The ./run --launcher -- --info-fd=1 ... approach doesn't seem to work since the stdout is never exhausted. Launching steam-runtime-launcher-service directly works as expected, though. This Python script can be used to reproduce it by running it inside the SteamLinuxRuntime_soldier installation directory:

from subprocess import Popen, PIPE

working_cmd = [
    "./pressure-vessel/bin/steam-runtime-launcher-service", "--info-fd=1", "--socket", "/tmp/test_steam_runtime.sock"
]
hanging_cmd = [
    "./run", "--launcher", "--pass-fd=1", "--", "--info-fd=1", "--socket", "/tmp/test_steam_runtime.sock"
]

try:
    process = Popen(hanging_cmd, stdout=PIPE)
    print("Starting")
    # Exhaust the stdout. Empty 'read(1)' result means EOF has been reached.
    while (byte := process.stdout.read(1)) != b"":
        print(byte.decode("utf-8"), end="")
    print("Started")
finally:
    process.terminate()

[smcv]

Hmm, thanks. That shouldn't be the case - I'll look at that when I get a chance.

I would guess that some other process in the hierarchy is keeping stdout open.

[smcv]

Ah, I think I see why that happens - it's probably because either pressure-vessel-adverb or the supervisor process in bwrap holds stdout open. I think I can see a route to preventing that in a future release of the container runtime, at which point it should be as simple as what I suggested.

(FYI, --pass-fd should never be required for fds 0, 1 or 2)

Until then, you could try something like this (untested):

read_end, write_end = os.pipe2(os.O_CLOEXEC)
proc = subprocess.Popen(
    [
        './run',
        '--launcher',
        '--pass-fd=%d' % write_end,
        '--',
        '--info-fd=%d' % write_end,
        '--bus-name=...',
    ],
    pass_fds=[write_end],
    stdin=subprocess.DEVNULL,
    stdout=..., # whatever you want
    stderr=..., # whatever you want
)

os.close(write_end)

with open(read_end, 'r') as reader:
        ignored = reader.read()

[Matoking]

Thank you, the suggested solution works as intended and I've pushed it to master.

[smcv]

Ah, I think I see why that happens - it's probably because either pressure-vessel-adverb or the supervisor process in bwrap holds stdout open. I think I can see a route to preventing that in a future release of the container runtime, at which point it should be as simple as what I suggested.

This was ValveSoftware/steam-runtime#593 and I fixed it in pressure-vessel 0.20230621.0, which is now in the default branch of SteamLinuxRuntime_soldier and SteamLinuxRuntime_sniper. That should allow you to use the simpler --info-fd=1 without an extra pipe, if you want to.

The extra pipe suggested in #231 (comment) is harmless, so you're welcome to keep using that - you don't need to change anything, switching to --info-fd=1 would just be a possible cleanup.

(--info-fd=1 isn't fixed in the previous_release branch yet, so if your users might be using that branch, wait a few weeks more for the fix to make its way into previous_release.)