Merge pull request #542 from qmfrederik/fixes/packfiles

GitPackMemoryCache: Don't have multiple callers reuse the same stream

Author: AArnott

src/NerdBank.GitVersioning.Tests/ManagedGit/GitPackMemoryCacheTests.cs

@@ -0,0 +1,41 @@
+using System.IO;
+using Nerdbank.GitVersioning.ManagedGit;
+using Xunit;
+
+namespace NerdBank.GitVersioning.Tests.ManagedGit
+{
+    /// <summary>
+    /// Tests the <see cref="GitPackMemoryCache"/> class.
+    /// </summary>
+    public class GitPackMemoryCacheTests
+    {
+        [Fact]
+        public void StreamsAreIndependent()
+        {
+            using (MemoryStream stream = new MemoryStream(
+                new byte[] { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 }))
+            {
+                var cache = new GitPackMemoryCache();
+
+                var stream1 = cache.Add(0, stream);
+                Assert.True(cache.TryOpen(0, out Stream stream2));
+
+                using (stream1)
+                using (stream2)
+                {
+                    stream1.Seek(5, SeekOrigin.Begin);
+                    Assert.Equal(5, stream1.Position);
+                    Assert.Equal(0, stream2.Position);
+                    Assert.Equal(5, stream1.ReadByte());
+
+                    Assert.Equal(6, stream1.Position);
+                    Assert.Equal(0, stream2.Position);
+
+                    Assert.Equal(0, stream2.ReadByte());
+                    Assert.Equal(6, stream1.Position);
+                    Assert.Equal(1, stream2.Position);
+                }
+            }
+        }
+    }
+}

src/NerdBank.GitVersioning/ManagedGit/GitPackDeltafiedStream.cs

@@ -93,6 +93,7 @@ public override int Read(Span<byte> span)
                 var source = instruction.InstructionType == DeltaInstructionType.Copy ? this.baseStream : this.deltaStream;
 
                 Debug.Assert(instruction.Size > this.offset);
+                Debug.Assert(source.Position + instruction.Size - this.offset <= source.Length);
                 canRead = Math.Min(span.Length - read, instruction.Size - this.offset);
                 didRead = source.Read(span.Slice(read, canRead));
 

src/NerdBank.GitVersioning/ManagedGit/GitPackMemoryCache.cs

@@ -7,28 +7,50 @@
 
 namespace Nerdbank.GitVersioning.ManagedGit
 {
-    internal class GitPackMemoryCache : GitPackCache
+    /// <summary>
+    /// <para>
+    ///   The <see cref="GitPackMemoryCache"/> implements the <see cref="GitPackCache"/> abstract class.
+    ///   When a <see cref="Stream"/> is added to the <see cref="GitPackMemoryCache"/>, it is wrapped in a
+    ///   <see cref="GitPackMemoryCacheStream"/>. This stream allows for just-in-time, random, read-only
+    ///   access to the underlying data (which may deltafied and/or compressed).
+    /// </para>
+    /// <para>
+    ///   Whenever data is read from a <see cref="GitPackMemoryCacheStream"/>, the call is forwarded to the
+    ///   underlying <see cref="Stream"/> and cached in a <see cref="MemoryStream"/>. If the same data is read
+    ///   twice, it is read from the <see cref="MemoryStream"/>, rather than the underlying <see cref="Stream"/>.
+    /// </para>
+    /// <para>
+    ///   <see cref="Add(long, Stream)"/> and <see cref="TryOpen(long, out Stream?)"/> return <see cref="Stream"/>
+    ///   objects which may operate on the same underlying <see cref="Stream"/>, but independently maintain
+    ///   their state.
+    /// </para>
+    /// </summary>
+    public class GitPackMemoryCache : GitPackCache
     {
-        private readonly Dictionary<long, Stream> cache = new Dictionary<long, Stream>();
+        private readonly Dictionary<long, GitPackMemoryCacheStream> cache = new Dictionary<long, GitPackMemoryCacheStream>();
 
+        /// <inheritdoc/>
         public override Stream Add(long offset, Stream stream)
         {
             var cacheStream = new GitPackMemoryCacheStream(stream);
             this.cache.Add(offset, cacheStream);
-            return cacheStream;
+            return new GitPackMemoryCacheViewStream(cacheStream);
         }
 
+        /// <inheritdoc/>
         public override bool TryOpen(long offset, [NotNullWhen(true)] out Stream? stream)
         {
-            if (this.cache.TryGetValue(offset, out stream))
+            if (this.cache.TryGetValue(offset, out GitPackMemoryCacheStream? cacheStream))
             {
-                stream.Seek(0, SeekOrigin.Begin);
+                stream = new GitPackMemoryCacheViewStream(cacheStream!);
                 return true;
             }
 
+            stream = null;
             return false;
         }
 
+        /// <inheritdoc/>
         public override void GetCacheStatistics(StringBuilder builder)
         {
             builder.AppendLine($"{this.cache.Count} items in cache");

src/NerdBank.GitVersioning/ManagedGit/GitPackMemoryCacheStream.cs

@@ -75,9 +75,9 @@ public override long Seek(long offset, SeekOrigin origin)
             {
                 var toRead = (int)(offset - this.cacheStream.Length);
                 byte[] buffer = ArrayPool<byte>.Shared.Rent(toRead);
-                this.stream.Read(buffer, 0, toRead);
+                int read = this.stream.Read(buffer, 0, toRead);
                 this.cacheStream.Seek(0, SeekOrigin.End);
-                this.cacheStream.Write(buffer, 0, toRead);
+                this.cacheStream.Write(buffer, 0, read);
                 ArrayPool<byte>.Shared.Return(buffer);
 
                 this.DisposeStreamIfRead();

src/NerdBank.GitVersioning/ManagedGit/GitPackMemoryCacheViewStream.cs

@@ -0,0 +1,77 @@
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Text;
+
+namespace Nerdbank.GitVersioning.ManagedGit
+{
+    internal class GitPackMemoryCacheViewStream : Stream
+    {
+        private readonly GitPackMemoryCacheStream baseStream;
+
+        public GitPackMemoryCacheViewStream(GitPackMemoryCacheStream baseStream)
+        {
+            this.baseStream = baseStream ?? throw new ArgumentNullException(nameof(baseStream));
+        }
+
+        public override bool CanRead => true;
+
+        public override bool CanSeek => true;
+
+        public override bool CanWrite => false;
+
+        public override long Length => this.baseStream.Length;
+
+        private long position;
+
+        public override long Position
+        {
+            get => this.position;
+            set => throw new NotSupportedException();
+        }
+
+        public override void Flush() => throw new NotImplementedException();
+
+        public override int Read(byte[] buffer, int offset, int count)
+        {
+            return this.Read(buffer.AsSpan(offset, count));
+        }
+
+#if NETSTANDARD
+        public int Read(Span<byte> buffer)
+#else
+        /// <inheritdoc/>
+        public override int Read(Span<byte> buffer)
+#endif
+        {
+            int read = 0;
+
+            lock (this.baseStream)
+            {
+                if (this.baseStream.Position != this.position)
+                {
+                    this.baseStream.Seek(this.position, SeekOrigin.Begin);
+                }
+
+                read = this.baseStream.Read(buffer);
+            }
+
+            this.position += read;
+            return read;
+        }
+
+        public override long Seek(long offset, SeekOrigin origin)
+        {
+            if (origin != SeekOrigin.Begin)
+            {
+                throw new NotSupportedException();
+            }
+
+            this.position = Math.Min(offset, this.Length);
+            return this.position;
+        }
+
+        public override void SetLength(long value) => throw new NotSupportedException();
+        public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException();
+    }
+}