When creating 20 files in FatFileSystem using new FatFileSystem per file created then "An item with the same key has already been added" exception is thrown

[henrikstengaard]

I stumbled upon an exception while creating a lot of single files using FatFileSystem where I create a new FatFileSystem instance for each file and managed to create following test to reproduce the issue:

    [Fact]
    public async Task CreateFilesInSubdirectoryUsingFatFileSystemPerFile()
    {
        using var diskStream = new SparseMemoryStream();

        diskStream.Position = 0;
        using (var fsFormat = FatFileSystem.FormatFloppy(diskStream, FloppyDiskType.HighDensity, "FLOPPY_IMG "))
        {
            fsFormat.CreateDirectory("dir");            
        }

        for (var i = 0; i < 20; i++)
        {
            using (var fsCreate = new FatFileSystem(diskStream))
            {
                using (var fileStream = fsCreate.OpenFile($"dir{Path.DirectorySeparatorChar}file{i}.txt", FileMode.Create))
                {
                    await fileStream.WriteAsync(new byte[10]);
                }
            }            
        }

        using var fsAssert = new FatFileSystem(diskStream);
        {
            var entries = fsAssert.GetFileSystemEntries("").ToList();
            Assert.Single(entries);
            Assert.Equal("dir", entries[0]);

            entries = fsAssert.GetFileSystemEntries("dir").ToList();
            Assert.Equal(20, entries.Count);
            Assert.Equal(Enumerable.Range(0, 20).Select(i => $"dir\\file{i}.txt"), entries);
        }
    }

From what I have managed to investigate so far, the issue seems related to ClusterStream.cs switching from one cluster to another when the cluster for dir is full (512 bytes) causing the position (pos) going from 512 to 0. The exception is then thrown by AddEntryRaw method adding the entry to _entries dictionary in Directory.cs and 0 already exists from an entry in previous cluster.

I would like to help with a fix for the issue, but I would also ask some advise. The _entries dictionary uses cluster position as key to index entries and must be changed to something else than ClusterStream position. I initially thought about using Guid's instead of long's as dictionary key, but then I realised the long key fromthe dictionary is also used to seek to entry positions when deleting entries. Here I'm not sure how seeking to position of entry to delete works when the cluster have changed within the ClusterStream. More investigation needed

[LTRData]

Interesting find! I do not remember exactly how this was implemented and I am not sure any of the original developers of that code are still around here. But I can probably help looking into what could be done about it in a few days or so!

[henrikstengaard]

That would be great. I investigated further and see the position within the ClusterStream must be used as it's a virtual position, so a guid is a no go. When the position goes beyond the first cluster, it should continue in the next cluster meaning when going from position 512 to 546 (512+32) switching from first to second cluster. Instead it goes from 512 to 0, which causes the directory class to reuse position 0 and tries to add position 0 as key to the _entries dictionary causing the exception because it already added the entry in first cluster at position 0.
It makes it more challenging to maintain without the original developers.

[henrikstengaard]

I have done more investigating and found the issue is _endOfEntries in the directory class is not updated when loading existing entries and the cluster is full and doesn't contain have a fat entry with end marker name.

In my test when creating the files, the cluster will at some point contain self and parent entry and file0-13.txt entry, but no end marker name entry. The entry file13.txt is at cluster position 480-512 and the cluster has the length 512 therefore end marker entry doesn't exist and _endOfEntries is not updated causing it to be 0 and will therefore start overwriting entries.

By discovering this, I modified load entries method to always update _endOfEntries with current position and this makes add entry allocate a new cluster continuing at position 512, 544, and so on instead of position 0 in the same cluster.

I will make a PR with the fix and tests.

[LTRData]

I will make a PR with the fix and tests.

Awesome! Thanks a lot!

By the way, not that long ago I made some changes to several code paths that updated cluster allocations etc. I had to do that when I added compatibility with other sector sizes than 512 bytes. But when doing that I also found some strange cases a bit similar to what you found now and it could be that I introduced a few more bugs at the same time.

[henrikstengaard]

I'm not sure if your changes would cause that, but I looked at the history for Directory.cs in Fat library and not much has changed there for the last couple of years. The most recent commits are mostly clean up.

[henrikstengaard]

PR #58 created with fix.