[FileEmulationFramework Deadlock Discussion]

[erock278]

Did you install the Prerequisites?
Yes.

Describe the Issue
When attempting to launch P5R from Reloaded II, multiple error messages pop up despite having followed the directions on gamebanana to the letter.

Screenshots

Result: Error message, when closed P5R opens and runs but without any mods.

Intended: No error message, P5R with mods.

[Sewer56]

Try clearing your cache by deleting Reloaded-II/Mods/p5rpc.modloader/Cache.

[erock278]

That didn't work, it gave me the same list of errors

[erock278]

Here is a list of all mods I have installed- it's not many, and about half are dependents for the others.

[Sewer56]

Can you hit Save Mod Set and upload that file here?
I'm curious myself.

Chances are it's a rare mod incompatibility resolveable by changing the enabled mods. The set would be useful in reproducing it however.

[erock278]

Absolutely
Mod Set 6 21 24.json

[erock278]

For reference I have tried running it in steam first, resetting my PC, readding P5R within Reloaded II, and closing and opening the game multiple times.

[Sewer56]

I'll CC @LTSophia and @AnimatedSwine37 who worked on that part of PersonaEssentials in the meantime.

If they can reproduce it, it'll hopefully be fixable.

Till then, just disable all the mods and re-enable them one by one. You'll know when things break.

[erock278]

Will do! I'll let you know if I get any more errors

[erock278]

Okay it looks like it's the Gy Joker mod that is causing the issue! I can activate all of them besides that (and the Gy Joker cheat sheet patch) without any issue. Should I maybe attempt to uninstall and reinstall it? That is the main reason I began modding in the first place lol. Also I am censoring because I am new on GitHub and don't want to be flagged/banned

[erock278]

oh the asterisks I was using as placeholders italicized the text, I should have seen that coming.

[Sewer56]

There's no censorship on this site.
In any case, I'll just let Swine/Sylvia look at it for the time being. I'm already busy as heck in general, as I always am.

[erock278]

Oh gotcha, good to know. Thanks for the timely help, I really really appreciate it! You seem like a very busy person, props to you. Should I message them as well to follow up?

[Sewer56]

They'll see it eventually. I won't worry about it too much :p

[erock278]

Alrighty! Thanks again man, take care!

[AnimatedSwine37]

I checked and I was able to reproduce the error which is good, I'll try and find a fix later today.

[AnimatedSwine37]

Alright, this is a weird edge case.

The gay Joker mod uses pak emulator on INIT\CMM.BIN\cmmHelp.bmd but Unhardcoded Romances (which is a dependency of it) uses it on init\cmm.bin\cmmHelp.bmd. This causes it to try and emulate the same file twice asynchronously, causing it to collide and explode (if done synchronously it's fine).

The game should be case insensitive so I've just changed all routes to uppercase so this doesn't happen and everything seems fine so far.

[AnimatedSwine37]

Actually, it isn't that simple, doing that causes some more edge cases where stuff won't work.
Specifically, I'm having trouble with dungeon save points. I think the problem is that there's an emulated bf and a non emulated file inside of an emulated pak, I think something about the emulated bf using different case for the pak causes only it to be included. I tried just making everything capital everywhere but then pak emulator itself isn't case sensitive when searching paks for files (possibly because paks themself aren't, not sure).

Anyway, maybe it could be fixed with code but it'll probably be a lot of work to make sure the changes don't break more stuff. For now I think we just get Cornflakes to change the case of that file in Unhardcoded Romances and generally encourage people to stick to the case that the game's file uses in the hopes that this just doesn't happen again.

[Sewer56]

This causes it to try and emulate the same file twice asynchronously, causing it to collide and explode (if done synchronously it's fine).

Did my recent change trigger this? @AnimatedSwine37

The one where I reduced the locks and asked PM to test, because some guy was hitting a deadlock due to (presumably driver induced) parallel reads with dependency.

Edit:

I ask because it doesn't repro on my end. So I couldn't look into this myself before.

[AnimatedSwine37]

Looks like it is, I just went back to 2.2.0 of File Emulation Framework Base Mod and it doesn't happen for me anymore.

[Sewer56]

Looks like it is, I just went back to 2.2.0 of File Emulation Framework Base Mod and it doesn't happen for me anymore.

Does it repro if you move the lock up to the loop which tries to create the emulators here?

https://github.com/Sewer56/FileEmulationFramework/blob/97be84b06b7687d7b7725294ea6720977a69192e/FileEmulationFramework/FileAccessServer.cs#L249-L262

That might be worth a try.
Though it risks deadlocking the person on Discord who caused me to be more lenient to locks in the first place.

[AnimatedSwine37]

Nope, it still happens

[Sewer56]

I'm guessing the lock might need lifting all the way up to Try Accept New File (virtual override) comment then.
And nested lock removed.

And if not that, well, diff time Sewer56/FileEmulationFramework@d9be97e

[AnimatedSwine37]

Nope, still get the error. To be sure though, this is what I did

lock (ThreadLock)
{
    // Try Accept New File (virtual override)
    if (PathToVirtualFileMap.TryGetValue(newFilePath, out var fileInfo))
    {
        // Reuse of emulated file (backed by stream) is safe because file access is single threaded.
        HandleToInfoMap[hndl] = new(fileInfo.FilePath, 0, fileInfo.File);
        PathToHandleMap[newFilePath] = hndl;
        return ntStatus;
    }

    // Try accept new file (emulator)
    for (var x = 0; x < Emulators.Count; x++)
    {
        var emulator = Emulators[x];
        if (!emulator.TryCreateFile(hndl, newFilePath, currentRoute.FullPath, out var emulatedFile))
            continue;

        HandleToInfoMap[hndl] = new(newFilePath, 0, emulatedFile);
        PathToHandleMap[newFilePath] = hndl;
        return ntStatus;
    }
}

[Sewer56]

Ah, pain.

I guess the only way around it is to diff then, see where the locks need moved to make this work.
I was honestly hoping NtCreateFile would have been enough; as it may have been cause of that deadlock the one guy had.

[AnimatedSwine37]

Adding the lock back around NtReadFileImpl seems to have fixed it.
https://github.com/Sewer56/FileEmulationFramework/blob/97be84b06b7687d7b7725294ea6720977a69192e/FileEmulationFramework/FileAccessServer.cs#L166

[Sewer56]

Adding the lock back around NtReadFileImpl seems to have fixed it.

Is that in conjunction with further locking emulator creation? (the code snippet above)

[AnimatedSwine37]

Nope, just adding back that one lock fixes it, I put back the NtCreateFileImpl locks back to how they were.

[Sewer56]

That might work. Would need to check with the guy back at https://discord.com/channels/746211612981198989/1252770490469056572/1252871602824282196 , non-zero chance it'll bring back their deadlock.

Also with moving the lock back up here. #399 (comment)
R2 emulators shouldn't spin extra threads when creating emus recursively, but the locks should technically be like in that snippet in case of concurrent access attempt by the game. Might be worth checking if that doesn't deadlock them either.

[Sewer56]

On another note, it might be worth debugging what's really going on here a bit more.

If you look at NtReadFile, the only 'artifact' it accesses that's modified elsewhere is HandleToInfoMap.
The caveat is that's modified via NtCreateFile. Until that function returns, you shouldn't really have a handle to a file you want to read.

So if locking that fixes up the problem; it does slightly concern me; because, that might mean we're using one handle to read from multiple threads at the same time. I'd consider that invalid use of the NtReadFile API (not just the hook, the general API), because then the read order is non-deterministic, and you wouldn't know where the read pointer would be moved to.

Here's an idea, make a HandleToThreadMap and update that map on entry/exit when in DEBUG mode i.e. #if DEBUG. If two threads are trying to use the same handle, fail with a MessageBox/Assert/Debugger.Launch or whatever fits the bill.

[Sewer56]

@AnimatedSwine37 Since I can't really reproduce this, would you be down for picking this up/investigating?

Reaching out to the 1 person who had the deadlock originally, seeing if the compromise works, and also having a debug check to see if multiple threads are hitting.