Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Fix/Workaround for sporadic 'IOS_FS: Failed to rename temporary FST file' issue on Windows. #9023

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AdmiralCurtiss
Copy link
Contributor

@AdmiralCurtiss AdmiralCurtiss commented Aug 11, 2020

So in case you've been unaware, there have been a handful of reports about this (one more) (one more), and I've personally also encountered it without much of a pattern. So I looked into this.

Unfortunately, I haven't really been able to figure out why this happens, but my best guess is the following:

  • Dolphin opens temp file and writes FST to it.
  • Dolphin closes temp file.
  • Antivirus/Dropbox/some other background process sees that a file was closed and opens it to check it.
  • While other process still has the file open, Dolphin attempts to rename it to its non-temp filename. This fails.

With that guess in mind, I began my search for a way to release the file only after it was renamed, and actually ended up finding something. If you open a file using CreateFileW() and request DELETE access, you can then rename the file with SetFileInformationByHandle() (see also) while keeping the handle open.

Now, I can't be sure this actually fixes the issue -- because it only occurs sporadically it's hard to test -- but I think it's worth a shot.

As for the code itself, this is more of a proof-of-concept than anything mergeable. We should probably discuss how we want to approach this. At the very least, this also needs a Linux impl (which can probably just forward to IOFile?), and in general probably should be a bit cleaner -- though I have no idea how to write that allocation of the FILE_RENAME_INFO structure without violating several C++ guidelines.

@AdmiralCurtiss AdmiralCurtiss force-pushed the failed-to-rename-fst-bin branch from df4d5c4 to 8b01027 Compare August 24, 2020 17:17
@shuffle2
Copy link
Contributor

on the forum link, someone recommends

Make sure you have only one instance of Dolphin running.

does dolphin really not work properly if multiple instances are running simultaneously on the same machine anymore (in general, not just related to this specific issue), or was this just some random guess at debugging the issue? If it's the former, that seems like a regression...

@shuffle2
Copy link
Contributor

on topic: I have seen this error on a machine which does not have any AV running (or anything I'd expect to be snooping files upon access). It would be nice to root cause the issue first.

@shuffle2
Copy link
Contributor

shuffle2 commented Aug 25, 2020

I did a quick look with procmon (just add a "path contains fst" filter and open dolphin / run some wii title), and yes, actually svchost hosting TrkWks service opens the file(s), as well as SearchProtocolHost process. I didn't receive an error, but maybe there is some race condition.
So, maybe a starting point to look into what is actually going on.

@JosJuice
Copy link
Member

does dolphin really not work properly if multiple instances are running simultaneously on the same machine anymore (in general, not just related to this specific issue), or was this just some random guess at debugging the issue? If it's the former, that seems like a regression...

I believe some parts of Dolphin won't work correctly if you are running two instances that are using the same User folder (or just the same NAND?) at the same time. If you set them to use separate User folders, then there is no problem.

@JMC47
Copy link
Contributor

JMC47 commented Sep 18, 2020

I started running into this recently, oddly enough after the font change made the GUI more responsive.

@leoetlino
Copy link
Member

Is this still useful now that #9250 has been merged?

@AdmiralCurtiss
Copy link
Contributor Author

I still think this is ultimately worth pursuing, but yeah, with that other PR merged this isn't super critical anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants