rfvgyhn / min-ed-launcher

Minimal Elite Dangerous Launcher
MIT License
251 stars 9 forks source link

Hangs forever on Steam flatpak, or whenever run without a controlling tty #74

Closed foresto closed 1 year ago

foresto commented 1 year ago

I had MinEdLauncher (v0.5.3, I think) working just fine with the flatpak version of Steam, but haven't played in several months.

Today, I upgraded MinEdLauncher to v0.7.5, mainly because the current flatpak freedesktop runtime no longer provides a .so file that the old MinEdLauncher build depends on.

Unfortunately, this new version just hangs when launched by Steam. It doesn't log anything. It doesn't create a new settings.json file to replace the old one I renamed. It doesn't launch the game. It just hangs forever, making Steam think the game is running when it is not.

If I press Steam's Stop button, and answer yes to the Exit Game prompt, Steam says "Stopping", but it never completes. Getting the process to stop requires that I hunt down the MinEdLauncher pid and kill it manually.

Here is the command being launched by Steam, as confirmed in both the running process list and Steam's terminal output:

./MinEdLauncher /home/myuser/.var/app/com.valvesoftware.Steam/.local/share/Steam/ubuntu12_32/reaper SteamLaunch AppId=359320 -- /home/myuser/.var/app/com.valvesoftware.Steam/.local/share/Steam/ubuntu12_32/steam-launch-wrapper -- /home/myuser/.var/app/com.valvesoftware.Steam/.local/share/Steam/steamapps/common/SteamLinuxRuntime_soldier/_v2-entry-point --verb=waitforexitandrun -- /home/myuser/.var/app/com.valvesoftware.Steam/.local/share/Steam/steamapps/common/Proton - Experimental/proton waitforexitandrun /home/myuser/.var/app/com.valvesoftware.Steam/.local/share/Steam/steamapps/common/Elite Dangerous/EDLaunch.exe /Steam /novr /autoquit /autorun /edh

None of these things helped:

No Proton log is created when I add PROTON_LOG=1 to the launch options. This, along with the fact that MinEdLauncher is neither creating a log file nor creating a default settings file, makes me think that it's hanging early, before it tries to do much at all.

If I enter a flatpak shell environment and manually run MinEdLauncher /dryrun, it creates a default settings.json file and logs this output:

2022-11-27 15:47:10.472 -08:00 [INF] Elite Dangerous: Minimal Launcher - v0.7.5+e47c3cd3
2022-11-27 15:47:10.602 -08:00 [DBG] 
    Args: [|"/dryrun"|]
    OS: Linux64
    Env: 

2022-11-27 15:47:10.606 -08:00 [DBG] Reading settings from '/home/myuser/.config/min-ed-launcher/settings.json'
2022-11-27 15:47:10.813 -08:00 [ERR] Unknown platform. Failed to find Elite Dangerous install directory

That shows that the program is capable of executing and creating its files in a flatpak environment, which is informative, but it doesn't solve the problem. I suspect the "unknown platform" error is just because it was launched outside of Steam.

Out of curiosity, I also manually ran the full command line that Steam uses (as quoted earlier). That logged a backtrace complaining about missing environment variables, which is unsurprising, since I didn't try to reproduce a whole Steam environment in my shell. It printed the same backtrace in the terminal window, followed by a "Press any key to quit..." prompt, and waited for me to press a key before it exited. If it's similarly waiting for user input when Steam launches it, perhaps that could explain why it hangs forever in Steam?

Of course, that doesn't explain why it hangs with no log output or config generation when launched by Steam.

foresto commented 1 year ago

After more investigation, I think these newer linux builds are assuming they will always be run with a controlling terminal, and hanging when that is not the case.

I tested this idea by writing a program that can launch another program with a pseudo-terminal attached. When I install this test program in my flatpak environment, and tell Steam to use it to launch MinEdLauncher, it runs without hanging.

I can't think of any reason why MinEdLauncher should require a controlling terminal. Printing text to stdout and stderr certainly doesn't require that. I now see that the README recommends using a terminal emulator, but there are at least two problems with that:

Since earlier versions didn't seem to have this problem, I guess maybe the change was an accident? Perhaps a side effect of using an F# library in a different way than it was used before? If so, I hope that change can be reverted.

pan-mroku commented 1 year ago

FWIW I observe no issues with launcher from git and steam from system repo.

I guess the dependency on console stems from this.

Are you sure you have no terminal? Not even xterm? I think I saw it inside steam's container. Here is how you can explore its environment.

rfvgyhn commented 1 year ago

The issue is Console.ReadLine(). https://github.com/rfvgyhn/min-ed-launcher/blob/e47c3cd3f55201cd3c243eeeec196370a835866f/src/MinEdLauncher/Program.fs#L52-L54

It hangs on any method (Peek, Read, etc...) that attempts to read the standard input stream. I haven't yet figured out why though. It shouldn't be waiting for user input when Console.IsInputRedirected (isatty(0) is 0), but that's what it appears to be doing.

This is almost the first thing the program does which is why there are no logs or settings creation. It was introduced in 72fa42c to support piping in Epic arguments from legendary which is why you didn't see the issue before v0.6.0.

rfvgyhn commented 1 year ago

A little more digging and it hangs at a system call within the .net std lib.

https://github.com/dotnet/runtime/blob/c2ec86b1c552ac8a1749f9f98e012f707e325660/src/libraries/Common/src/Interop/Unix/System.Native/Interop.Read.cs#L4-L23

The generated code looks like

[System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Interop.LibraryImportGenerator", "7.0.7.1805")]
internal static unsafe partial int Read(global::System.Runtime.InteropServices.SafeHandle fd, byte* buffer, int count)
{
    int __lastError;
    System.IntPtr __fd_native = default;
    int __retVal;
    // Setup - Perform required setup.
    bool fd__addRefd = false;
    try
    {
        // Marshal - Convert managed data to native data.
        fd.DangerousAddRef(ref fd__addRefd);
        __fd_native = fd.DangerousGetHandle();
        {
            System.Runtime.InteropServices.Marshal.SetLastSystemError(0);
            __retVal = __PInvoke(__fd_native, buffer, count);
            __lastError = System.Runtime.InteropServices.Marshal.GetLastSystemError();
        }
    }
    finally
    {
        // Cleanup - Perform required cleanup.
        if (fd__addRefd)
            fd.DangerousRelease();
    }

    System.Runtime.InteropServices.Marshal.SetLastPInvokeError(__lastError);
    return __retVal;
    // Local P/Invoke
    [System.Runtime.InteropServices.DllImportAttribute("libSystem.Native", EntryPoint = "SystemNative_Read", ExactSpelling = true)]
    static extern unsafe int __PInvoke(System.IntPtr fd, byte* buffer, int count);
}

and it hangs at the call to __PInvoke.

I assume it has to do with whatever the contents (if anything) of the input stream is. The bad news is, I don't really know where to go from here.

rfvgyhn commented 1 year ago

After reading the man page for read, it looks like maybe this is an issue with an empty stream.

If a process attempts to read from an empty pipe, then read(2) will block until data is available.

Perhaps there's an API I'm not aware of to handle this in a "nice" way.

pan-mroku commented 1 year ago

What about checking return of read or checking for keypress?

foresto commented 1 year ago

[Console.ReadLine] shouldn't be waiting for user input when Console.IsInputRedirected (isatty(0) is 0), but that's what it appears to be doing.

I don't know how IsInputRedirected is implemented, but I verified that isatty(0) is returning a false value.

It occurs to me that a redirected stdin doesn't necessarily mean it's redirected to a closed/empty stream. It could be connected to a pipe that just doesn't have any data available yet, for example. In that case, wouldn't Console.ReadLine be expected to block?

Perhaps what's needed here is an accurate way of detecting Epic, rather than just assuming its presence and expecting incoming data whenever stdin happens to be redirected?

foresto commented 1 year ago

FYI, adding </dev/null to the end of the Steam launch options avoids the hang. This seems to support the hypothesis in my comment above.

foresto commented 1 year ago

After reading the man page for read, it looks like maybe this is an issue with an empty stream.

If a process attempts to read from an empty pipe, then read(2) will block until data is available.

Yes, that is normal behavior for I/O functions like read() when operating on a file descriptor that was opened in blocking mode, which is the default. Very standard stuff in unix and C.

Perhaps there's an API I'm not aware of to handle this in a "nice" way.

You seem to be hinting at non-blocking I/O, which is certainly available on unix using C, though I don't know if dotnet exposes it. The most common APIs for this are select() and poll(), and there are a variety of OS-specific ones built for performance. I would avoid that approach, though, since I think it's more complicated than necessary here. Better to simply not read from a redirected stdin unless we know what is at the other end and what data to expect from it.

rfvgyhn commented 1 year ago

What about checking return of read or checking for keypress?

That doesn't work because there's no data to read which causes the read call to block.

[Console.ReadLine] shouldn't be waiting for user input when Console.IsInputRedirected (isatty(0) is 0), but that's what it appears to be doing.

I don't know how IsInputRedirected is implemented, but I verified that isatty(0) is returning a false value.

As far as I can tell, it's just isatty(0) == 0 (on linux anyway).

It occurs to me that a redirected stdin doesn't necessarily mean it's redirected to a closed/empty stream. It could be connected to a pipe that just doesn't have any data available yet, for example. In that case, wouldn't Console.ReadLine be expected to block?

Yes, it's expected. I guess I just didn't think it through when I was stepping through the std lib.

Perhaps what's needed here is an accurate way of detecting Epic, rather than just assuming its presence and expecting incoming data whenever stdin happens to be redirected?

Knowing if the user launching via Epic isn't really the problem since it passes in unique arguments. The only way to authenticate an Epic game license is to use a one-time code the Epic launcher generates which it passes in as an argument to the process (-auth_password). Since legendary has already reverse engineered and implemented that code, I choose to use that instead of learning and implementing it myself. Not sure how to get at that without piping it and reading from stdin.

foresto commented 1 year ago

In that case, what about detecting Legendary?

rfvgyhn commented 1 year ago

Yes, that is normal behavior for I/O functions like read() when operating on a file descriptor that was opened in blocking mode, which is the default. Very standard stuff in unix and C.

Yeah, this topic is pretty new to me.

You seem to be hinting at non-blocking I/O, which is certainly available on unix using C, though I don't know if dotnet exposes it. The most common APIs for this are select() and poll(), and there are a variety of OS-specific ones built for performance. I would avoid that approach, though, since I think it's more complicated than necessary here.

I was just actually reading about select and poll. I haven't found any exposure to that in .net's std lib, but there is a library that looks like it includes poll.

In that case, what about detecting Legendary?

Do you mean having the min launcher launch legendary if it's available?

foresto commented 1 year ago

Do you mean having the min launcher launch legendary if it's available?

I hadn't thought of that, but I guess it might be worth considering. Could that work?

I actually meant checking whether our parent process is a Legendary instance, and reading from stdin only in that case.

However, I just looked over the relevant parts of our README, and while it looks like that could work when following the Windows instructions, I now see that the Linux instructions use a shell pipeline that would not make Legendary the parent process (or even keep it running long enough to detect).

What about adding a /legendary command line option to tell MinEdLauncher it should read from stdin?

Or, rather than reading from stdin, what about using the shell's $() command substitution feature to pass Legendary output as a command line argument to MinEdLauncher? Something like this:

MinEdLauncher /legendary $(legendary launch --dry-run 9c203b6ed35846e8a4a9ff1e314f6593 | grep "Launch parameters" | cut -d : -f 3-) /autorun /autoquit /edh

I like the former idea for its simplicity, but the latter one would slightly simplify the existing Linux instructions (and avoid bashisms).

rfvgyhn commented 1 year ago

Command substitution is a good idea. Not dealing with stdin would also mean getting rid of a regex. Win win.

foresto commented 1 year ago

Not dealing with stdin would also mean getting rid of a regex. Win win.

Agreed, although I don't know if a $() equivalent is available on Windows. Historically, it was not, but I haven't kept up on recent Windows shell features.

rfvgyhn commented 1 year ago

Windows users are able to use legendary's override-exe argument since they don't have to deal with a wine environment, so it should be all good.

Removed stdin support will be in the next release. You can use the CI build until then if you'd like.

Thanks for the detailed report, the suggestions and teaching me a bit more about linux. :)

foresto commented 1 year ago

Thanks for being a good collaborator. :) I look forward to the next release.