dokan-dev / dokan-dotnet

Dokan DotNet Wrapper
http://dokan-dev.github.io
MIT License
462 stars 116 forks source link

Remounted volumes are randomly unmounted after a few seconds #347

Closed Noire001 closed 10 months ago

Noire001 commented 11 months ago

Hi, my project manages multiple Dokan volumes so I have a class that holds DokanInstance objects in a dictionary when they are mounted. When unmounting a volume, the corresponding DokanInstance is also removed from the dictionary. However when mounting the same volume again using a new DokanInstance (but same mountpoint), the volume is randomly unmounted after a while.

Consider the following Program.cs for mirror sample:

internal class Program
    {
        private static readonly Dictionary<string, DokanInstance> _dokanInstances =
            new Dictionary<string, DokanInstance>();

        private static readonly Dokan Dokan = new Dokan(new ConsoleLogger("[Dokan] "));

        private static async Task Main(string[] args)
        {
            MountMirror("N");
            // unmount mirror after 2 seconds
            Task.Run(async () =>
            {
                await Task.Delay(2000);
                Dokan.Unmount('N');
            });
            await _dokanInstances["N"].WaitForFileSystemClosedAsync(uint.MaxValue);
            _dokanInstances.Remove("N");

            MountMirror("N");
            await _dokanInstances["N"].WaitForFileSystemClosedAsync(uint.MaxValue);
            Console.WriteLine("Program shouldn't reach here");
        }

        private static void MountMirror(string letter)
        {
            var mirror = new Mirror(new ConsoleLogger("[Mirror] "), "C:\\");

            var dokanBuilder = new DokanInstanceBuilder(Dokan)
                .ConfigureLogger(() => new ConsoleLogger("[Dokan] "))
                .ConfigureOptions(options =>
                {
                    options.Options = DokanOptions.DebugMode;
                    options.MountPoint = letter + ":\\";
                });
            var dokanInstance = dokanBuilder.Build(mirror);
            _dokanInstances.Add(letter, dokanInstance);
        }
    }

Mirror is mounted, unmounted after 2 seconds then mounted again. After a few more seconds mirror is randomly unmounted without calling Dokan.Unmount a second time.

Also sometimes I get this exception:

Unhandled Exception: System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
   at DokanNet.Native.NativeMethods.DokanCloseHandle(IntPtr dokanInstance)
   at DokanNet.DokanHandle.ReleaseHandle()
   at System.Runtime.InteropServices.SafeHandle.InternalFinalize()
   at System.Runtime.InteropServices.SafeHandle.Finalize()

Process finished with exit code -1,073,741,819.

This only happens when re-mounting with the same mount point. If I call MountMirror the second time with a different mountpoint there are no issues. There's also no issue if I add all DokanInstance objects to a list and keep them there forever.

I think this is what happens:

  1. Volume is unmounted, first DokanInstance is removed from dictionary
  2. Second volume is mounted at same mountpoint, second DokanInstance added to dictionary
  3. First DokanInstance is garbage collected, unexpectedly unmounting the second volume
Liryna commented 11 months ago

Hi @Noire001,

How does the C# and native logs looks like when this happens ?

Noire001 commented 11 months ago

Attaching logs

dokan-dotnet.txt dokan-tracespy.txt

Liryna commented 11 months ago

Thanks. That's interesting. I believe NativeMethods.DokanCloseHandle should not be called on a device that was dismount externally. DokanInstance should prevent that somehow. @LTRData any idea how we could achieve this in C# ? @

LTRData commented 11 months ago

Thanks. That's interesting. I believe NativeMethods.DokanCloseHandle should not be called on a device that was dismount externally. DokanInstance should prevent that somehow. @LTRData any idea how we could achieve this in C# ? @

You are right, this is not currently handled in a safe way for dismounting externally and later calls to DokanCloseHandle from within application itself. From what I can see, this could be an issue when using only native API as well, depending on exact workflow. The issue basically seems to be that RemoveMountPoint needs to be guarded somehow so that it is not called again, potentially for another drive. The native DokanInstance structure still needs to be freed though? So, we should either guard RemoveMountPoint calls with a mutex and verify that the drive letter still points to the expected drive, or use some other mechanism to check whether drive letter has already been removed.

Noire001 commented 11 months ago

Forgive my naivety, but why not call DokanCloseHandle immediately when unmounting?

Here's what I tried: Setting ownsHandle to false in DokanHandle constructor prevents it from calling ReleaseHandle when finalized:

public DokanHandle() : base(ownsHandle: false)

Calling DokanCloseHandle in a new Unmount method (since it also unmounts the volume anyway):

        public void Unmount(DokanInstance instance)
        {
            NativeMethods.DokanCloseHandle(instance.DokanHandle.DangerousGetHandle());
        }

This seems to fix the issue


Also a small observation:

https://github.com/dokan-dev/dokan-dotnet/blob/3eea6739a7de42759aa91bce0861cbadecb825dc/DokanNet/DokanInstance.cs#L87

This does not seem to call DokanCloseHandle. The implementation of Dispose in SafeHandle (of which DokanHandle is derived) is this:

    public void Dispose() => this.Dispose(true);

    protected virtual void Dispose(bool disposing)
    {
      if (disposing)
        this.InternalDispose();
      else
        this.InternalFinalize();
    }

The Dispose method will call InternalDispose. However looking at the stack trace in OP, ReleaseHandle is called from InternalFinalize.

If we are supposed to call DokanInstance.Dispose() immediately after an unmount, then perhaps overriding Dispose in DokanHandle and calling DokanCloseHandle always would also work.

LTRData commented 11 months ago

Yes, DokanCloseHandle should be called from DokanHandle.ReleaseHandle. That is how Dispose pattern is supposed to work in .NET, because the SafeHandle derived object that holds the actual native object should be the one that frees the native object, nothing else should. Otherwise, safe handle reference counting does not work, and everything gets very risky.

Also, all IDisposable objects should be disposed by user code, otherwise you end up with cleanup called after a while by a finalizer as you see there. When that happens, it means that there is a missed Dispose call somewhere in your code. So, if you make sure that you really dispose all DokanInstance objects and DokanInstance.Dispose calls DokanHandle.Dispose and that in turn calls DokanCloseHandle, then we can remove the confusing DokanInstance.Unmount method and everything should work as expected.

Liryna commented 11 months ago

I correct myself (native), it is safe to call DokanCloseHandle on a device that was RemoveMountPoint. The DokanInstance still exists and actually DokanCloseHandle is the only function (outside DokanMain but that’s not used here) that can free the instance.

DokanCloseHandle should be called after WaitForFileSystemClosedAsync somewhere if the unmount does not come from DokanInstance.Unmount or remove it so DokanInstance.Dispose does the job as @LTRData said above.

Noire001 commented 11 months ago

Okay, disposing DokanInstance (after unmount) does actually call ReleaseHandle and it solves my issue. Don't know why but I tried everything except this. Thanks!

Liryna commented 11 months ago

Thanks @Noire001 for the confirmation! Should we do anything here to prevent this (code wise) or just close the issue ?