Added access to SteamAPI_RestartAppIfNecessary

[Unsanctioned]

Steam recommends calling this at app start (before SteamAPI_Init) if you aren't using their DRM.

[Ventero]

Sorry for the delay in reviewing this. Looks pretty good, thanks!

Just two requests:

• Could you add a test for the new function (maybe one that only runs if some command line parameter is passed, so that the case where Steam isn't running can also be tested)?
• Any chance you could rebase your branch against master? The history is a bit cluttered right now...

[Unsanctioned]

I can do that. Wasn't sure what to do for the test, since that function will kill the app and restart it through Steam. A command-line option sounds like a good alternative.

I should be able to look at those at the start of the week.

On May 2, 2014, at 2:12 PM, Ventero notifications@github.com wrote:

Sorry for the delay in reviewing this. Looks pretty good, thanks!

Just two requests:

Could you add a test for the new function (maybe one that only runs if some command line parameter is passed, so that the case where Steam isn't running can also be tested)? Any chance you could rebase your branch against master? The history is a bit cluttered right now... \ Reply to this email directly or view it on GitHub.

[Unsanctioned]

It looks like the rebase attempt completely broke the branch. I'll see what I can do to recover it.

[Ventero]

I think the main problem is that your master branch is diverged from my upstream branch. The following steps should help in cleaning up the history (make sure you save/stash any unsaved changes).

# assuming you added my repository as "upstream" remote
git fetch upstream
# switch to your master branch
git checkout master
# this will throw away any uncomitted changes in your working tree and reset your master branch to the state of my upstream master branch
git reset --hard upstream/master
# optionally, push this branch to your github remote
# git push -f origin master
# now switch to your feature-branch
git checkout feature-appstart
# do an interactive rebase, in the text editor window that pops up, delete all lines that contain commits unrelated to the appstart feature (you can also squash/edit the remaining commits, if you want)
git rebase -i master
# the rebase should finish without any conflicts, which you can verify now. then, overwrite the remote branch with your current history.
git push -f origin feature-appstart

[Ventero]

PS: I pushed the commits as they were a few minutes ago into a feature-appstart branch in my repository. If the above procedure doesn't work, you can just run

git fetch upstream
git checkout feature-appstart
git reset --hard upstream/feature-appstart

to reset your branch to that state.

[Unsanctioned]

Sounds good. Since you have those changes in master now, it shouldn't be any problem for me to just reset my fork to upstream. The conflict seemed to be with the double addition of the MicroTransaction changes.

[Ventero]

I've looked at the code now, and it looks mostly good (some minor nitpicks like bracing style/trailing whitespace). Again, thank you so much for your contribution!

However, the Linux implementation doesn't work at all if calling restartAppIfNecessary before init, as the wrapper process is only started during the init call.

Unfortunately I added the APIWrapper binary path as an argument to init() instead of the constructor (which in retrospect was a really bad idea), so we can't just start the binary during the call to restartAppIfNecessary, as it might not be placed at the default path. I guess the easiest solution is just to move the path argument to the constructor as a backwards-incompatible change, and do a version bump ...

Any thoughts about this? Maybe I'm missing an obvious solution for this problem ...

[Unsanctioned]

I suppose we could expose a new method to explicitly start the wrapper process on Linux, which can be called before init or restartAppIfNecessary. The init function on Linux would then only start the process if not already started.

That would remain backwards-compatible for cases where restartAppIfNecessary isn't used. We can then document the APIWrapper path argument of the init function as deprecated to encourage switching to the new method of starting the wrapper process on Linux.

On May 4, 2014, at 8:03 AM, Ventero notifications@github.com wrote:

I've looked at the code now, and it looks mostly good (some minor nitpicks like bracing style/trailing whitespace). Again, thank you so much for your contribution!

However, the Linux implementation doesn't work at all if calling restartAppIfNecessary before init, as the wrapper process is only started during the init call.

Unfortunately I added the APIWrapper binary path as an argument to init() instead of the constructor (which in retrospect was a really bad idea), so we can't just start the binary during the call to restartAppIfNecessary, as it might not be placed at the default path. I guess the easiest solution is just to move the path argument to the constructor as a backwards-incompatible change, and do a version bump ...

Any thoughts about this? Maybe I'm missing an obvious solution for this problem ...

\ Reply to this email directly or view it on GitHub.

[Ventero]

That's a very good idea, thanks! Do you want to add that, or should I do it? I also left two nitpick comments on the diff, but otherwise looks good to me, thanks!

[Unsanctioned]

I'm traveling today, but I can add that tomorrow. I'll put that in a separate branch and pull request from the restartAppIfNecessary changes, since it's a dependency for the Linux build and can work separately.

On May 5, 2014, at 1:34 PM, Ventero notifications@github.com wrote:

That's a very good idea, thanks! Do you want to add that, or should I do it? I also left two nitpick comments on the diff, but otherwise looks good to me, thanks!

\ Reply to this email directly or view it on GitHub.

[Unsanctioned]

The linux initialization change pull request is submitted. If that's acceptable, I can merge it with the restartAppIfNecessary change and resubmit that pull request with the change included.

[Ventero]

Oh, sorry, I merged that commit before reading your comment here. :)

So I guess if you add a commit to this which adds a call to startProcess in the Linux implementation of restartAppIfNecessary (and maybe fix the two nitpicks), I will merge this as well.

[Unsanctioned]

The two nitpicks are fixed up. I moved the test for restartAppIfNecessary to after the linux process start call in the test harness after integrating the linux branch to the feature-appstart branch.

I don't think we can call startProcess from restartAppIfNecessary, since that would then require passing the APIWrapper path to the function on linux. I think it should be enough to document that calling startProcess() is necessary prior to restartAppIfNecessary() on Linux.

On May 6, 2014, at 3:10 PM, Ventero notifications@github.com wrote:

Oh, sorry, I merged that commit before reading your comment here. :)

So I guess if you add a commit to this which adds a call to startProcess in the Linux implementation of restartAppIfNecessary (and maybe fix the two nitpicks), I will merge this as well.

— Reply to this email directly or view it on GitHub.

[Ventero]

Mh, I think that's a bit risky, as in case someone forgets to call startProcess explicitly, restartAppIfNecessary will return false (i.e. "Steam is running").

So in my opinion, it would make more sense to require the user to call startProcess only if they use a non-standard path for the wrapper. Otherwise, just call startProcess in restartAppIfNecessary and assume they want to use the default path.

Either way, we definitely need to document the requirement to call startProcess explicitly when using a non-default path somewhere.

[Unsanctioned]

Hm. What if restartAppIfNecessary throws an exception if the process isn't started instead of returning false?

Attempting to start the process using the default path would result in the same problem you describe if the process doesn't start correctly.

[Ventero]

Mh, I'm not sure about throwing an exception ...

My reasoning is that using a non-default path for the APIWrapper binary is an edge case (I'm not sure if anyone actually uses a non-default path), and I want to make using restartAppIfNecessary as simple as possible for the default case. Thus, those using the default path shouldn't have to do anything special - like calling startProcess - for that function to work, especially uf there's no explicit reason why they would have to do that.

Thus, I'd say restartAppIfNecessary should try to start the process with a default path (if not already running), and only bail out if that doesn't work either - similar to what init does. For the same reason I'm not sure if throwing an exception is a good idea , as init doesn't throw one if the path is incorrect (maybe it should?). But I guess throwing is definitely preferrable over returning "success" (i.e. false) if the process startup failed.

[Unsanctioned]

How about this: if the process isn't running, restartAppIfNecessary() will try to start it with the default path. If that fails, it throws an error.

As long as init returns false when the process can't be started, I don't think that one needs to throw an error since a return value of false means init failed.

[Ventero]

Yeah, I can agree with that.

The problem with init returning false is that the value of false can have several meanings: Steam isn't running, the app id couldn't be determined (e.g. when starting the game directly instead of through Steam) or the wrapper couldn't be started.

The first two of these problems are due to user error and can't be avoided by the developer, whereas the last one is a general problem with the packaging setup of the application. Thus, it would make sense to warn the developer explicitly about this problem by throwing an exception, instead of simply returning false. But I guess that's outside the scope of this PR.

[Unsanctioned]

Added those updates. I also added a check to ensure init() has not already been called, since SteamAPI_RestartAppIfNecessary can't be called after SteamAPI_Init.

[Ventero]

I'm trying to test the case where restartAppIfNecessary returns true right now and it seems to me that when the test application exits immediately after checking the return value, Steam is actually never started. Wrapping the call to exit() in a setTimeout(..., 1000); solves this problem though - and so does an extra call to runCallbacks before exiting. Can you confirm this?

Since this is an easy thing to miss, I think this call should also be added to restartAppIfNecessary.

Oh, and one thing I just noticed: Since restartAppIfNecessary requires a special implementation for Linux (and Windows/OS X, if adding the runCallbacks call), you'll also have to add it to the blacklist in lib/generateAPI.rb, otherwise it will be overwritten the next time that script is run.

[Ventero]

Oh, and you'll also have to move the function above the "// START GENERATED CODE" line.

... I really need better documentation about this whole process.

[Unsanctioned]

I ran into something like that in my testing as well. In a mxml app, I couldn't exit out during the applicationComplete call and had to wait until the first invoke event.

Does adding runCallbacks() to restartAppIfNecessary fix it for you, without the delay? Can you call runCallbacks() if the Steam API hasn't been initialized?

[Ventero]

Yeah, adding a runCallbacks call fixes it even without delay (which makes sense, as the only thing that a delay does is giving the callback timer a chance to fire).

Calling runCallbacks directly calls SteamAPI_RunCallbacks() in the native part without going through g_Steam, so it should be safe to call it before initializing the API. The docs don't say anything about having to call init first either, and in my test it seems to work fine.

[Ventero]

Apologies, it looks like runCallback was actually a red herring. The problem seems to be that when restartAppIfNecessary returns true, the Steam API library creates a new child process of the calling process (which in this case is the APIWrapper binary), and in that execve()s something like /bin/sh -c steam.sh steam://run/480 &. For whatever reason, this process then segfaults under certain conditions.

I have no idea what might be causing this though, as starting the APIWrapper binary directly and manually calling restartIfNecessary (echo -e '8\n480' | LD_PRELOAD=$STEAM_SDK/redistributable_bin/linux32/libsteam_api.so NativeApps/Linux/APIWrapper) works just fine.

I'll investigate this further tomorrow. Sorry for all the trouble with this PR ...

[Ventero]

Okay, before I forget all of this again until tomorrow:

The SIGSEGV was actually yet another red herring caused by strace.

What actually happens is that when the Steam API lib forks the APIWrapper process to start the Steam client in a child process, it inherits all the file descriptors from the APIWrapper process - including stdout and stderr. At some point after the Steam API forked that process, the AIR runtime is told to exit, so it shuts down the APIWrapper process and closes its stdout/stderr (which are pipes handled by the AIR process).

If at some point after this shutdown is complete, the child process created by the Steam API tries to write to the now-closed stdout/-err, it triggers a SIGPIPE signal. The default action for this signal is to quit, effectively aborting the Steam startup.

There's three possible solutions for this: Either try to keep the AIR process (and thus the APIWrapper's stdout/-err pipes) alive long enough for the child process to finish, fix up the stdout/stderr file descriptors, or simply ignore the SIGPIPE (i.e. signal(SIGPIPE, SIG_IGN);).

I think I prefer the last solution ... which, admittedly, is an ugly hack - but it's far from the only one in the APIWrapper. I also think that this is something that should be handled by the Steam API lib, so I guess I'll report this problem upstream.

[Ventero]

I've cherry-picked the first 3 commits of this pull request and included a fix for the SIGPIPE issue on Linux.

Thank you for your patience with this PR, and as always, your contributions are much appreciated!