Fix caching test (#1855)

* Make sure we always dispose the stream * Actually test for the attachment content
getsentry · Aug 17, 2022 · ef06390 · ef06390
1 parent 4c662d7
commit ef06390
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 1 deletion.
diff --git a/src/Sentry/Envelopes/Envelope.cs b/src/Sentry/Envelopes/Envelope.cs
@@ -189,6 +189,11 @@ public static Envelope FromEvent(
                         }
                         else
                         {
+                            // We would normally dispose the stream when we dispose the envelope item
+                            // But in this case, we need to explicitly dispose here or we will be leaving
+                            // the stream open indefinitely.
+                            stream.Dispose();
+
                             logger?.LogWarning("Did not add '{0}' to envelope because the stream was empty.",
                                 attachment.FileName);
                         }

diff --git a/test/Sentry.Tests/Internals/Http/CachingTransportTests.cs b/test/Sentry.Tests/Internals/Http/CachingTransportTests.cs
@@ -32,12 +32,13 @@ public async Task WithAttachment()
             FileSystem = _fileSystem
         };
 
+        string httpContent = null;
         Exception exception = null;
         var innerTransport = new HttpTransport(options, new HttpClient(new CallbackHttpClientHandler(async message =>
          {
              try
              {
-                 await message.Content!.ReadAsStringAsync();
+                 httpContent = await message.Content!.ReadAsStringAsync();
              }
              catch (Exception readStreamException)
              {
@@ -47,8 +48,15 @@ public async Task WithAttachment()
 
         await using var transport = CachingTransport.Create(innerTransport, options, startWorker: false);
 
+        const string attachmentContent = "test-attachment";
         var tempFile = Path.GetTempFileName();
 
+#if NETCOREAPP
+        await File.WriteAllTextAsync(tempFile, attachmentContent);
+#else
+        File.WriteAllText(tempFile, attachmentContent);
+#endif
+
         try
         {
             var attachment = new Attachment(AttachmentType.Default, new FileAttachmentContent(tempFile), "Attachment.txt", null);
@@ -59,6 +67,7 @@ public async Task WithAttachment()
             await transport.FlushAsync();
 
             // Assert
+            Assert.Contains(attachmentContent, httpContent);
             if (exception != null)
             {
                 throw exception;

diff --git a/test/Sentry.Tests/Protocol/Envelopes/EnvelopeTests.cs b/test/Sentry.Tests/Protocol/Envelopes/EnvelopeTests.cs
@@ -721,6 +721,25 @@ public void FromEvent_EmptyAttachmentStream_DoesNotIncludeAttachment()
         envelope.Items.Should().HaveCount(1);
     }
 
+    [Fact]
+    public void FromEvent_EmptyAttachmentStream_DisposesStream()
+    {
+        // Arrange
+        var path = Path.GetTempFileName();
+        using var stream = File.OpenRead(path);
+        var attachment = new Attachment(
+            default,
+            new StreamAttachmentContent(stream),
+            "Screenshot.jpg",
+            "image/jpg");
+
+        // Act
+        _ = Envelope.FromEvent(new SentryEvent(), attachments: new List<Attachment> { attachment });
+
+        // Assert
+        Assert.Throws<ObjectDisposedException>(() => stream.ReadByte());
+    }
+
     [Fact]
     public async Task Serialization_RoundTrip_ReplacesSentAtHeader()
     {