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

[BUG] BlobClient.UploadAsync(Stream content, bool overwrite) deadlocks if the stream is not rewinded #14080

Closed
luckerby opened this issue Aug 10, 2020 · 14 comments · Fixed by #14301
Labels
bug This issue requires a change to an existing behavior in the product in order to be resolved. Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. needs-team-attention This issue needs attention from Azure service team or SDK team Service Attention This issue is responsible by Azure service team. Storage Storage Service (Queues, Blobs, Files)
Milestone

Comments

@luckerby
Copy link

luckerby commented Aug 10, 2020

Consider the following code, where stream is valid and previously created, and pos is a number larger than 0 and less than or equal to the stream's size:

stream.Seek(pos, SeekOrigin.Begin);

// Write to Azure Blob Storage
BlobClient blobClient = blobContainerClient.GetBlobClient("MyBlob1");
await blobClient.UploadAsync(stream, true);

If this is run, the code will deadlock while awaiting. The Tasks view in Visual Studio shows multiple tasks running:
image

Upon analyzing, each task N is waiting for task N-1, with the exception of:

  • 36 (waiting on 161)
  • 141 (waiting on 173)
  • 168 (only scheduled, and waiting to run)

The call stack, while breaking into during the stall, is below:
image

Using try/catch around the async call doesn't result in anything throwing.

Should the position in the stream be 0, the call completes as expected.

Environment:

  • Name and version of the Library package used: Azure.Storage.Blobs 12.4.4
  • Hosting platform or OS and .NET runtime version (dotnet --info output for .NET Core projects): Windows 10 .NET Core 3.1.0
  • IDE and version : Visual Studio 16.5.0 Preview 2
@msftbot msftbot bot added needs-triage This is a new issue that needs to be triaged to the appropriate team. customer-reported Issues that are reported by GitHub users external to the Azure organization. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels Aug 10, 2020
@jsquire jsquire added Client This issue points to a problem in the data-plane of the library. needs-team-attention This issue needs attention from Azure service team or SDK team Service Attention This issue is responsible by Azure service team. Storage Storage Service (Queues, Blobs, Files) labels Aug 10, 2020
@msftbot msftbot bot removed the needs-triage This is a new issue that needs to be triaged to the appropriate team. label Aug 10, 2020
@msftbot
Copy link

msftbot bot commented Aug 10, 2020

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @xgithubtriage.

@jsquire
Copy link
Member

jsquire commented Aug 10, 2020

Thank you for your feedback. Tagging and routing to the team best able to assist.

@amnguye
Copy link
Member

amnguye commented Aug 11, 2020

Hi,

I was able to reproduce your issue. I was able to put a Fiddler trace on it too, looks like the packet will take a while to upload before it realizes that it's missing the bytes from where the position is moved. So we send the Content-Length to be the size of the stream but unfortunately we hand it the bytes minus the position moved. So then we get a 408 (Request Body Incomplete) and we attempt to retry since its a 408 which is why you might be seeing a deadlock cause it takes a very long time.

We will look to resolve this issue when we can.

Notes:
reproduce with

public async Task UploadAsync_Stream()
        {
            await using DisposingContainer test = await GetTestContainerAsync();

            var name = GetNewBlobName();
            BlobClient blob = InstrumentClient(test.Container.GetBlobClient(name));
            var data = GetRandomBuffer(Constants.KB);

            using (var stream = new MemoryStream(data))
            {
                **stream.Seek(4, SeekOrigin.Begin);**
                await blob.UploadAsync(stream);
            }

            System.Collections.Generic.IList<BlobItem> blobs = await test.Container.GetBlobsAsync().ToListAsync();
            Assert.AreEqual(1, blobs.Count);
            Assert.AreEqual(name, blobs.First().Name);

            Response<BlobDownloadInfo> download = await blob.DownloadAsync();
            using var actual = new MemoryStream();
            await download.Value.Content.CopyToAsync(actual);
            TestHelper.AssertSequenceEqual(data, actual.ToArray());
        }

Fiddler Trace Request (This gets retried since its a 408)

PUT https://[REDACTED]/test-container-adc38975-7ccd-47c5-a385-bd4b4f799b61/test-blob-2fa02b7b-c1af-4c22-8307-ef23102e5494 HTTP/1.1
x-ms-blob-type: BlockBlob
x-ms-version: 2019-12-12
If-None-Match: *
x-ms-client-request-id: a215f50a-a8f6-4f8e-933c-748404c05d41
x-ms-return-client-request-id: true
User-Agent: azsdk-net-Storage.Blobs/12.5.0 (.NET Core 4.6.29017.01; Microsoft Windows 10.0.18363 )
x-ms-date: Tue, 11 Aug 2020 22:31:42 GMT
Authorization: [REDACTED]
Content-Length: 1024
Host: [REDACTED]

Fiddler Trace Response

HTTP/1.1 408 Request body incomplete
Date: Tue, 11 Aug 2020 22:32:42 GMT
Content-Type: text/html; charset=UTF-8
Connection: close
Cache-Control: no-cache, must-revalidate
Timestamp: 15:32:42.634

The request body did not contain the specified number of bytes. Got 1,020, expected 1,024

Stack Trace

Message: 
    System.Xml.XmlException : Data at the root level is invalid. Line 1, position 1.
  Stack Trace: 
    XmlTextReaderImpl.Throw(Exception e)
    XmlTextReaderImpl.Throw(String res, String arg)
    XmlTextReaderImpl.ParseRootLevelWhitespace()
    XmlTextReaderImpl.ParseDocumentContent()
    XmlTextReaderImpl.Read()
    XDocument.Load(XmlReader reader, LoadOptions options)
    XDocument.Load(Stream stream, LoadOptions options)
    BlockBlob.UploadAsync_CreateResponse(ClientDiagnostics clientDiagnostics, Response response) line 12915
    BlockBlob.UploadAsync(ClientDiagnostics clientDiagnostics, HttpPipeline pipeline, Uri resourceUri, Stream body, Int64 contentLength, String version, Nullable`1 timeout, Byte[] transactionalContentHash, String blobContentType, String blobContentEncoding, String blobContentLanguage, Byte[] blobContentHash, String blobCacheControl, IDictionary`2 metadata, String leaseId, String blobContentDisposition, String encryptionKey, String encryptionKeySha256, Nullable`1 encryptionAlgorithm, String encryptionScope, Nullable`1 tier, Nullable`1 ifModifiedSince, Nullable`1 ifUnmodifiedSince, Nullable`1 ifMatch, Nullable`1 ifNoneMatch, String ifTags, String requestId, String blobTagsString, Boolean async, String operationName, CancellationToken cancellationToken) line 12726
    ValueTask`1.get_Result()
    BlockBlobClient.UploadInternal(Stream content, BlobHttpHeaders blobHttpHeaders, IDictionary`2 metadata, IDictionary`2 tags, BlobRequestConditions conditions, Nullable`1 accessTier, IProgress`1 progressHandler, String operationName, Boolean async, CancellationToken cancellationToken) line 718
    <<GetPartitionedUploaderBehaviors>b__1>d.MoveNext() line 2033
    --- End of stack trace from previous location where exception was thrown ---
    PartitionedUploader`2.UploadInternal(Stream content, TServiceSpecificArgs args, IProgress`1 progressHandler, Boolean async, CancellationToken cancellationToken) line 171
    BlobClient.StagedUploadInternal(Stream content, BlobUploadOptions options, Boolean async, CancellationToken cancellationToken) line 1240
    BlobClient.UploadAsync(Stream content) line 331
    BlobClientTests.UploadAsync_Stream() line 156
    BlobClientTests.UploadAsync_Stream() line 166
    GenericAdapter`1.GetResult() line 99
    AsyncToSyncAdapter.Await(Func`1 invoke) line 60
    TestMethodCommand.Execute(TestExecutionContext context) line 64
    <>c__DisplayClass1_0.<Execute>b__0() line 58
    BeforeAndAfterTestCommand.RunTestMethodInThreadAbortSafeZone(TestExecutionContext context, Action action) line 73

@amnguye amnguye added bug This issue requires a change to an existing behavior in the product in order to be resolved. and removed question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels Aug 11, 2020
@seanmcc-msft
Copy link
Member

seanmcc-msft commented Aug 17, 2020

@pakrym, what would you think if we attempted to mitigate this with:

if (stream.CanSeek && stream.Position >= stream.Length)
{
    throw new ArgumentException($"{nameof(stream)}.{nameof(stream.Position)} must be less than {nameof(stream)}.{nameof(stream.Length)}");
}

@pakrym
Copy link
Contributor

pakrym commented Aug 17, 2020

@pakrym, what would you think if we attempted to mitigate this with:

I don't think that this mitigation would work. stream.Position >= stream.Length would always be true for a MemoryStream.

@seanmcc-msft
Copy link
Member

seanmcc-msft commented Aug 17, 2020

Are you sure? What about if a MemoryStream was just created, or it's position was set to less than stream.Length?

using NUnit.Framework;
using System;
using System.IO;

namespace ConsoleApp31
{
    class Program
    {
        static void Main(string[] args)
        {
            byte[] array = new byte[1024];
            Stream stream = new MemoryStream(array);

            Assert.IsTrue(stream.Position < stream.Length);

            Console.WriteLine(stream.Position); // 0
            Console.WriteLine(stream.Length);   // 1024
        }
    }
}

@pakrym
Copy link
Contributor

pakrym commented Aug 17, 2020

Sorry, it will always be false. And I'm not sure how it would mitigate the problem, passing a stream with a non-zero position is completely valid.

@seanmcc-msft
Copy link
Member

seanmcc-msft commented Aug 17, 2020

I think stream.Position == stream.Length after the stream as been fully read:

        [Test]
        public async Task AppendBlockAsync()
        {
            await using DisposingContainer test = await GetTestContainerAsync();

            // Arrange
            var blobName = GetNewBlobName();
            AppendBlobClient blob = InstrumentClient(test.Container.GetAppendBlobClient(blobName));
            await blob.CreateAsync();
            const int blobSize = Constants.KB;
            var data = GetRandomBuffer(blobSize);

            // Act
            using (var stream = new MemoryStream(data))
            {
                await blob.AppendBlockAsync(stream);

                Assert.IsTrue(stream.Position == stream.Length);

                // Deadlock
                await blob.AppendBlockAsync(stream);
            }
        }

The check I previously mentioned would ensure we are working with a valid position [0, stream.Length).

@pakrym
Copy link
Contributor

pakrym commented Aug 17, 2020

Can you please explain how your proposal fixes the issue customers are seeing?

@seanmcc-msft
Copy link
Member

seanmcc-msft commented Aug 17, 2020

It solves a common edge case of this issue, when stream.Position == stream.Length. We could make the error message more detailed, asking the customer to rewind the stream.

I think its possible the deadlock when 0 < Stream.Position < Stream.Length might be in Azure Core. It repos on our simple upload methods, where we are mostly calling the generated code:

        [Test]
        public async Task AppendBlockAsync()
        {
            await using DisposingContainer test = await GetTestContainerAsync();

            // Arrange
            var blobName = GetNewBlobName();
            AppendBlobClient blob = InstrumentClient(test.Container.GetAppendBlobClient(blobName));
            await blob.CreateAsync();
            const int blobSize = Constants.KB;
            var data = GetRandomBuffer(blobSize);

            // Act
            using (var stream = new MemoryStream(data))
            {
                stream.Position = 5;

                // Deadlock
                await blob.AppendBlockAsync(stream);
            }
        }

The other possibilities are it could be something in NonDisposingStream or StorageProgressExtensions.

@pakrym
Copy link
Contributor

pakrym commented Aug 17, 2020

@kasobol-msft had found the source of the issue already, it's because

doesn't take Position into account when setting a Content-Lenght header value. Fixing math there and in similar spots would fix the bug.

@seanmcc-msft
Copy link
Member

seanmcc-msft commented Aug 17, 2020

Understood, I will create a PR to address this throughout the Storage SDK. I still think it would be valuable to add the stream.Position < stream.Length check, this is a common case that customers run into.

@pomtom
Copy link

pomtom commented Dec 21, 2021

This issue still exist in Azure.Storage.Blobs 12.10.0 ?

@amnguye
Copy link
Member

amnguye commented Dec 21, 2021

Hi, please provide reproduction of the problem you are running into using 12.10.0 with as much details as possible so we can resolve it.

Then we can determine if we need to reopen this issue or open a different one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue requires a change to an existing behavior in the product in order to be resolved. Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. needs-team-attention This issue needs attention from Azure service team or SDK team Service Attention This issue is responsible by Azure service team. Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants