Improve eject() crashes on DX12

[zed0]

This PR reduces eject crashes by restricting the Minhook disable/uninitialize to the end of the Present call. This prevents it happening at a time when Present is wanting to use the resources that are being unhooked.

I'm fully expecting to make a lot of changes to this code, but thought I'd make a draft PR in case you have any advice on whether this is a reasonable approach.
I'm not very happy with the way synchronisation between the unhook thread and the present thread is done, but it was easy for a proof of concept.

This changes the number of crashes I get on eject while playing Shadow of the Tomb Raider with DX12 from ~30% of the time to 0% (so far).

[veeenu]

Hey, thank you so much for your contribution!

This is great and it would be great to have such a synchronization mechanism for all other backends too. Busy looping is probably not the way to go although arguably it'll only ever briefly happen once at eject which could result in a brief stutter (1s in tragic situations where you just can't get the thread running the unapply). But I am afraid that it could result in a deadlock in single threaded games.

An alternative mechanism which could be a bit more lightweight could be to actually switch to a single atomic boolean, something along the lines of PRESENT_BUSY that gets flipped true every time we enter Present and false just before the return (but not before calling the original -- we'd have to store its return value, flip the bool, and return the value). Once that's in place, some other caller could check for it and conditionally run the eject. This might still result in a brief delay, but at least it won't block any thread with a busy loop.

Another alternative yet could be to just have eject flip its own EJECT_REQUESTED atomic bool to true when called, and have the decommissioning of all resources happen at the end of Present, after having called the original function. This seems sound to me as I believe no two threads are going to be inside of Present at the same time (barring weird situations with multiple renderers active concurrently?), which, if it is true, means that we can guarantee that the next call after we leave that function will be to the old function, and we won't be running into weird shenanigans where the jump is changed on the fly.

[zed0]

The code has been updated to replace busy loops with the EJECT_REQUESTED approach as suggested. This runs most of the ejection code synchronously during Present. The DLL ejection then happens in a separate thread which waits for none of the hooked functions to be running before ejecting the DLL.
The code is certainly a lot cleaner without the loops 😅

[artosimonyan]

Hey @veeenu, think this will solve the issues mentioned in #139, which I'm also experiencing? If so can we get it reviewed and merged if possible?

Edit: Just to confirm, using this, I haven't seen a single crash so far, where it was crashing regularly previously during unload.

[veeenu]

Hi,

I haven't forgotten this, I've just been impossibly busy and couldn't really devote any time at all to open source lately.
I'll try to carve some time in the coming week.

[veeenu]

Thank you for this!

I tested this against darksoulsiii-practice-tool and eldenring-practice-tool and it works flawlessly every time. I would just like a bit of extra clarity on one point, but otherwise LGTM.

And sorry it took a long time, but it's been hard to carve spare time lately.

[veeenu]

I don't understand why the lint workflow is stuck. Hopefully a new run will kick off if we add one or more commits?

[Godnoken]

Haven't had the chance to try this out, but thank you for this, looks like it is working flawlessly! :)

[zed0]

I don't understand why the lint workflow is stuck. Hopefully a new run will kick off if we add one or more commits?

Github is telling me that you need to approve the running of the lint step

[veeenu]

LGTM, thank you so much for this 🙌

Some of the lint errors are newer ones that come from the library itself and not this commit, so I fixed those and took the chance to bump the version number so we can issue a new release.

Feel free to rebase and address the few ones remaining, then we can merge in!

[veeenu]

LGTM, thank you!