Fixes for multi-user session issues

dokan-dev / dokany

User mode file system library for windows with FUSE Wrapper

http://dokan-dev.github.io

5.21k stars 661 forks source link

Fixes for multi-user session issues #1236

Closed LTRData closed 1 month ago

LTRData commented 1 month ago

Ref discussion: https://github.com/dokan-dev/dokany/discussions/1235

Checklist

[X] My code follows the style guidelines of this project
[X] I have performed a self-review of my own code
[x] I have commented my code, particularly in hard-to-understand areas
[ ] I have made corresponding changes to the existing documentation
[X] My changes generate no new warnings
[ ] I have updated the change log (Add/Change/Fix)
[X] I have cleaned up the commit history (use rebase and squash)

LTRData commented 1 month ago

I have implemented fixes for issues discussed https://github.com/dokan-dev/dokany/discussions/1235, including calling FsRtlRegisterUncProvider and FsRtlDeregisterUncProvider directly from requestor thread instead of a system thread.

I hope this looks good. I have ran lots of successful tests in multi-user environments now to verify that everything works as expected.

Liryna commented 1 month ago

Interesting the test failed for UNC on Appveyor image 2015 but worked on the 2017 https://ci.appveyor.com/project/Maxhy/dokany/builds/50299852/job/d97jyx1t9mdu93sr

Maybe with the above change (PsTerminateSystemThread) it will pass

LTRData commented 1 month ago

Fixed the issues. As a sidenote, for another discussion, I believe the other RunAsSystem calls can be changed to specifying global namespace as part of path to IoCreateSymbolicLink instead.

LTRData commented 1 month ago

Test still fail. Even the tests that are marked as successful (such as 2017) are actually failing. It shows in the logs, although they for some reason summarize the failures in a different way.

So, I'll revert the RunAsSystem changes for now and try to find out more about how and why that change caused failures!

Liryna commented 1 month ago

Yeah that's surprising 😢 Agree we can look at this later

LTRData commented 1 month ago

Tests successful. I think it all looks good now.

Liryna commented 1 month ago

Yeap looks good! Thank @LTRData for the contribution 👍 I will try to make a release ASAP if you need a release

LTRData commented 1 month ago

Yeap looks good! Thank @LTRData for the contribution 👍 I will try to make a release ASAP if you need a release

Thanks! That would be great! 👍