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

[C#] Cannot delete fasterlog from disk if the log was disposed with a commit request in flight #417

Closed
hiteshmadan opened this issue Mar 4, 2021 · 2 comments · Fixed by #423

Comments

@hiteshmadan
Copy link
Contributor

hiteshmadan commented Mar 4, 2021

I recently upgraded to FASTER's latest nuget version and the following testcase started failing.

The testcase initiates a write, initiates a commit (with spinWait=false), disposes off the log and the device, and then tries to delete the log from disk. This can easily happen in my app because the app has API's to create/delete logs and to append to logs. The delete and append can be done concurrently.

The delete fails with System.IO.IOException: The process cannot access the file 'commit.0.0' because it is being used by another process.

I figured out that there is some code in fasterlog that's holding an active reference to the log-commit file even after dispose.

Note that if I remove the call to log.Commit, or do the commit with spinWait=true, then the delete does not fail.

Note that even if i wait for the commit to complete (with spinWait=false) - by checking that the ValueTask returned by TryEnqueue has succeeded/failed - I still cannot delete the log.

[TestMethod]
public void TestDisposeReleasesFileLocksWithInprogressCommit()
{
    DirectoryInfo di = Directory.CreateDirectory(this.TestContext.TestRunDirectory + $"\\log");
    IDevice device = Devices.CreateLogDevice(this.TestContext.TestRunDirectory + $"\\log\\testDisposeReleasesFileLocksWithInprogressCommit.log", preallocateFile: true, deleteOnClose: false);
    FasterLog fasterLog = new FasterLog(new FasterLogSettings { LogDevice = device, LogChecksum = LogChecksumType.PerEntry });
    fasterLog.TryEnqueue(new byte[100], out long beginAddress).Should().BeTrue();
    fasterLog.Commit(spinWait: false);
    fasterLog.Dispose();
    device.Dispose();
    di.Delete(recursive: true);
}
@badrishc
Copy link
Contributor

badrishc commented Mar 5, 2021

Commit initiates a write to disk, so the delete would fail until that write completes. The following seems to work:

        [Test]
        public void TestDisposeReleasesFileLocksWithInprogressCommit()
        {
            var commitPath = TestContext.CurrentContext.TestDirectory + "/" + TestContext.CurrentContext.Test.Name + "/";
            DirectoryInfo di = Directory.CreateDirectory(commitPath);

            IDevice device = Devices.CreateLogDevice(commitPath + "fasterlog.log", preallocateFile: true, deleteOnClose: false);
            FasterLog fasterLog = new FasterLog(new FasterLogSettings { LogDevice = device, LogChecksum = LogChecksumType.PerEntry });
            Assert.IsTrue(fasterLog.TryEnqueue(new byte[100], out long beginAddress));
            fasterLog.Commit(spinWait: false);
            fasterLog.WaitForCommitAsync().GetAwaiter().GetResult();
            fasterLog.Dispose();
            device.Dispose();
            di.Delete(recursive: true);
        }

Note that even if i wait for the commit to complete (with spinWait=false) - by checking that the ValueTask returned by TryEnqueue has succeeded/failed - I still cannot delete the log.

TryEnqueue does not return a ValueTask. If you meant EnqueueAsync then note that this task completes when the record has been successfully enqueued in memory, not when it is committed to disk:

        /// <summary>
        /// Enqueue entry to log in memory (async) - completes after entry is 
        /// appended to memory, NOT committed to storage.
        /// </summary>
        /// <param name="entry">Entry to enqueue</param>
        /// <param name="token">Cancellation token</param>
        /// <returns></returns>
        public ValueTask<long> EnqueueAsync(byte[] entry, CancellationToken token = default)

@hiteshmadan
Copy link
Contributor Author

hiteshmadan commented Mar 5, 2021

Note that even if i wait for the commit to complete (with spinWait=false) - by checking that the ValueTask returned by TryEnqueue has succeeded/failed - I still cannot delete the log.

I misspoke here, i meant to say "by checking that the ValueTask returned by WaitForCommitAsync has faulted".

Here is something that fails (delete fails with IOException) even if I wait for the commit task to fail. Adding a 10 second delay before deleting the subscription also doesn't free up the file lock, so it looks unlikely there's a race condition in how the testcase is structured ?

[TestMethod]
public async Task TestDisposeReleasesFileLocksWithInprogressCommit()
{
    DirectoryInfo di = Directory.CreateDirectory(this.TestContext.TestRunDirectory + $"\\log");
    IDevice device = Devices.CreateLogDevice(this.TestContext.TestRunDirectory + $"\\log\\testDisposeReleasesFileLocksWithInprogressCommit.log", preallocateFile: true, deleteOnClose: false);
    FasterLog fasterLog = new FasterLog(new FasterLogSettings { LogDevice = device, LogChecksum = LogChecksumType.PerEntry });
    fasterLog.TryEnqueue(new byte[100], out long beginAddress).Should().BeTrue();
    fasterLog.Commit(spinWait: false);
    ValueTask waitingWriter = fasterLog.WaitForCommitAsync(beginAddress + 1);
    fasterLog.Dispose();
    device.Dispose();
    while (!waitingWriter.IsCompleted)
    {
        await Task.Yield();
    }
    waitingWriter.IsFaulted.Should().BeTrue();
    await Task.Delay(10_000);

    di.Delete(recursive: true);
}

@badrishc badrishc changed the title Cannot delete fasterlog from disk if the log was disposed with a commit request in flight [C#] Cannot delete fasterlog from disk if the log was disposed with a commit request in flight Mar 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants