[Bug Report] GetFileUploadSasUriAsync throws ObjectDisposedException

[JohanNorberg]

Context

• OS, version, SKU and CPU architecture used: Both Windows & Linux
• Application's .NET Target Framework : .net core 3.1
• Device: Desktop, Web App
• SDK version used:

The bug is in 1.38, 1.39
Everything worked as it should in earlier versions, tested: 1.37.2, 1.37, 1.36, 1.35

Description of the issue

The SDK version 1.38 introduced issues when uploading files using DeviceClient, with Pooling and Amqp_Tcp_Only

To reproduce (100% reproduceable, on both Linux and Windows):

1. Create a DeviceClient for Device A, do a file upload (call GetFileUploadSasUriAsync, then upload to blob, then call CompleteFileUploadAsync)
2. Dispose DeviceClient for Device A.
3. Create a DeviceClient for Device B, do a fileupload
4. Dispose DeviceClient for Device B
5. Create a DeviceClient for Device A, do a file upload, (will throw on GetFileUploadSasUriAsync)

Code sample exhibiting the issue

you will need a textfile called "example.txt" in the same folder.

This will throw when using SDK version 1.38 & 1.39 but NOT on earlier versions.

class Program
    {
        static ExponentialBackoff retryPolicy
            = new ExponentialBackoff(int.MaxValue, TimeSpan.FromMilliseconds(100), TimeSpan.FromSeconds(10), TimeSpan.FromMilliseconds(100));

        static async Task Main(string[] args)
        {
            var tasks = new List<Task>();
            for (int i = 1; i < 10; i++)
            {
                int clientIndex = i % 2 == 0 ? 1 : 2;
                var task = Task.Run(async () => await RunClient(clientIndex));
                await task;
                tasks.Add(task);
            }

            await Task.WhenAll(tasks);

            Console.WriteLine("All Done");
        }

        private static async Task RunClient(int clientIndex)
        {
            using (
            var client = DeviceClient
            .CreateFromConnectionString("insertconnectionstringhere",
                $"testissues_{clientIndex}",
                new ITransportSettings[] {
                    new AmqpTransportSettings(TransportType.Amqp_Tcp_Only)
                    {
                        AmqpConnectionPoolSettings = new AmqpConnectionPoolSettings()
                        {
                            Pooling = true
                        }
                    }})
            )
            {
                client.SetRetryPolicy(retryPolicy);
                Twin twin = await client.GetTwinAsync();

                Console.WriteLine($"Connected client : {clientIndex}");

                for (int i = 0; i < 2; i++)
                {
                    string filename = $"dontdispose/test{i}.txt";
                    Console.WriteLine(filename);
                    var filelocation = await client.GetFileUploadSasUriAsync(new FileUploadSasUriRequest { BlobName = filename },
                        new CancellationTokenSource(1000 * 30).Token);
                    var uri = filelocation.GetBlobUri();

                    Console.WriteLine(JsonConvert.SerializeObject(filelocation));
                    Console.WriteLine(uri);

                    using (var httpClient = new HttpClient())
                    {
                        httpClient.DefaultRequestHeaders.Add("x-ms-blob-type", "BlockBlob");
                        await httpClient.PutAsync(uri, new ByteArrayContent(await File.ReadAllBytesAsync("example.txt")));
                    }

                    Console.WriteLine("File uploaded");

                    await client.CompleteFileUploadAsync(new FileUploadCompletionNotification
                    {
                        CorrelationId = filelocation.CorrelationId,
                        IsSuccess = true,
                        StatusCode = 201,
                        StatusDescription = "successfully uploaded file"
                    });

                    await Task.Delay(100);
                }
            }
        }
    }

Console log of the issue

Unhandled exception. System.ObjectDisposedException: Cannot access a disposed object.
Object name: 'The authentication method instance has already been disposed, so this client is no longer usable. Please close and dispose your current client instance. To continue carrying out operations from your device/ module, create a new authentication method instance and use it for reinitializing your client.'.
   at Microsoft.Azure.Devices.Client.AuthenticationWithTokenRefresh.GetTokenAsync(String iotHub)
   at Microsoft.Azure.Devices.Client.Transport.HttpClientHelper.ExecuteAsync(HttpMethod httpMethod, Uri requestUri, Func`3 modifyRequestMessageAsync, Func`2 isSuccessful, Func`3 processResponseMessageAsync, IDictionary`2 errorMappingOverrides, CancellationToken cancellationToken)
   at Microsoft.Azure.Devices.Client.Transport.HttpClientHelper.PostAsync[T1,T2](Uri requestUri, T1 entity, IDictionary`2 errorMappingOverrides, IDictionary`2 customHeaders, CancellationToken cancellationToken)
   at Microsoft.Azure.Devices.Client.Transport.HttpTransportHandler.GetFileUploadSasUriAsync(FileUploadSasUriRequest request, CancellationToken cancellationToken)
   at IoTHubUploadFile.Program.RunClient(Int32 clientIndex) in C:\Work\Elux\IoTHubUploadFile\IoTHubUploadFile\Program.cs:line 60
   at IoTHubUploadFile.Program.<>c__DisplayClass1_0.<<Main>b__0>d.MoveNext() in C:\Work\Elux\IoTHubUploadFile\IoTHubUploadFile\Program.cs:line 25
--- End of stack trace from previous location where exception was thrown ---
   at IoTHubUploadFile.Program.Main(String[] args) in C:\Work\Elux\IoTHubUploadFile\IoTHubUploadFile\Program.cs:line 26
   at IoTHubUploadFile.Program.<Main>(String[] args)

[azabbasi]

@JohanNorberg
Thanks for reaching out to us.
I have run your sample code multiple times and I can't seem to reproduce the issue as you describe it. I tried 1.39, 1.38 and they both ran fine.
The error you get indicates that you are using the DeviceClient after it has been disposed but I don't see that issue with the code snippet you shared with us.
Did you by any chance fix it before sending it our way? just speculating at this point.

I did notice some issues with your code though that don't seem to be related to the issue you are hitting but will affect the quality of the code.

1-

static async Task Main(string[] args)
        {
            var tasks = new List<Task>();
            for (int i = 1; i < 10; i++)
            {
                int clientIndex = i % 2 == 0 ? 1 : 2;
                var task = Task.Run(async () => await RunClient(clientIndex));
                await task;
                tasks.Add(task);
            }

            await Task.WhenAll(tasks);

            Console.WriteLine("All Done");
        }

Can be simplified to :

        static async Task Main(string[] args)
        {
            for (int i = 1; i < 10; i++)
            {
                int clientIndex = i % 2 == 0 ? 1 : 2;
                await RunClient(clientIndex);
            }

            Console.WriteLine("All Done");
        }

Your main method is async Task so you can directly await async methods in main.

2- Creating a new HTTP client every time you want to send a request is ill-advised. It will result in connection issues and port exhaustion. You should either create a static HTTP client and reuse it or instead use the Azure Storage SDK as illustrated in our sample code for file upload notification

3- It's recommended to close your device client once you are done with it. The using statement will Dispose it but to properly close everything, you should call await client.CloseAsync(); Once you are done with the client.

[JohanNorberg]

Hey, ok, sorry about the messy code. I just experimented until I could reproduce the error and then submitted with the code I had. I should have cleaned up before submitting the bug report. I've cleaned up the code here:

class Program
    {
        static HttpClient httpClient = new HttpClient();

        static async Task Main(string[] args)
        {
            httpClient.DefaultRequestHeaders.Add("x-ms-blob-type", "BlockBlob");

            await Test("testissues_7", "test1.txt");
            await Test("testissues_8", "test2.txt");
            await Test("testissues_7", "test3.txt");

            Console.WriteLine("All Done, if you see this, it's very weird..");
        }

        public static async Task Test(string deviceId, string filename)
        {
            Console.WriteLine($"Test - Device: {deviceId}, Filename: {filename}");

            using (var deviceClient = DeviceClient.CreateFromConnectionString("HostName=t...=",
               deviceId, new ITransportSettings[] {
                    new AmqpTransportSettings(TransportType.Amqp_Tcp_Only)
                    {
                        AmqpConnectionPoolSettings = new AmqpConnectionPoolSettings()
                        {
                            Pooling = true
                        }
                    }}))
            {
                // Get twin, this is normally done at start. For this test we don't need the data for anything but
                // it forces a connection and makes sure that everything is working correctly.
                // Note that if you don't have this line, everything will be working correctly.
                await deviceClient.GetTwinAsync();

                // Init the file upload
                var file = await deviceClient.GetFileUploadSasUriAsync(new FileUploadSasUriRequest { BlobName = filename });

                // Upload the file
                await httpClient.PutAsync(file.GetBlobUri(), new ByteArrayContent(Encoding.UTF8.GetBytes("Hello World")));

                // Complete the file upload
                await deviceClient.CompleteFileUploadAsync(new FileUploadCompletionNotification
                {
                    CorrelationId = file.CorrelationId,
                    IsSuccess = true,
                    StatusCode = 201,
                    StatusDescription = "hello world"
                });

                await deviceClient.CloseAsync();
            }
        }
    }

The output is as follows (using SDK 1.39):

Test - Device: testissues_7, Filename: test1.txt
Test - Device: testissues_8, Filename: test2.txt
Test - Device: testissues_7, Filename: test3.txt
Unhandled exception. System.ObjectDisposedException: The authentication method instance has already been disposed, so this client is no longer usable. Please close and dispose your current client instance. To continue carrying out operations from your device/ module, create a new authentication method instance and use it for reinitializing your client.
Object name: 'DeviceAuthenticationWithSakRefresh'.
   at Microsoft.Azure.Devices.Client.AuthenticationWithTokenRefresh.GetTokenAsync(String iotHub)
   at Microsoft.Azure.Devices.Client.Transport.HttpClientHelper.ExecuteAsync(HttpMethod httpMethod, Uri requestUri, Func`3 modifyRequestMessageAsync, Func`2 isSuccessful, Func`3 processResponseMessageAsync, IDictionary`2 errorMappingOverrides, CancellationToken cancellationToken)
   at Microsoft.Azure.Devices.Client.Transport.HttpClientHelper.PostAsync[T1,T2](Uri requestUri, T1 entity, IDictionary`2 errorMappingOverrides, IDictionary`2 customHeaders, CancellationToken cancellationToken)
   at Microsoft.Azure.Devices.Client.Transport.HttpTransportHandler.GetFileUploadSasUriAsync(FileUploadSasUriRequest request, CancellationToken cancellationToken)
   at IoTHubUploadFile.Program.Test(String deviceId, String filename) in C:\Work\Elux\IoTHubUploadFile\IoTHubUploadFile\Program.cs:line 49
   at IoTHubUploadFile.Program.Main(String[] args) in C:\Work\Elux\IoTHubUploadFile\IoTHubUploadFile\Program.cs:line 25
   at IoTHubUploadFile.Program.<Main>(String[] args)

However, downgrading to 1.37.2 gives the following output:

Test - Device: testissues_7, Filename: test1.txt
Test - Device: testissues_8, Filename: test2.txt
Test - Device: testissues_7, Filename: test3.txt
All Done, if you see this, it's very weird..

[azabbasi]

I can't repro the issue you are coming across for some reason. I tried all mutations of windows/linux and 1.39 and 1.38 SDK versions.
I think the next step would be for you to capture the SDK logs and share them with us so we can inspect what is happening in depth. You can capture the SDK logs by following this document.
You can either send it directly to me or share them however you feel comfortable.
[email protected]

[azabbasi]

@JohanNorberg
Hi Johan,
Thanks for the call earlier today. We have identified the root cause of the issue and here is a quick summary:
This issue only arises when you are using the following combination:
AMQP + Connection Pooling + Group SAS Authentication

As discussed today, we highly recommend using device SAS authentication instead of group SAS authentication. If your SAS token gets leaked in one device your whole IoT hub is compromised.
Also, you are using connection pooling but it technically doesn't matter in your scenario as you are disposing the client before the next device client gets instantiated so you can avoid using connection pooling.

Having said all that, there is a bug in the SDK where the initial CBS link doesn't get disposed which is causing the behavior you see. We are working on a fix and it will be out soon.

So to work around the issue, we recommend the following:

• Do not use connection pooling if you don't have to (your device clients get initialized and disposed consecutively so you don't need connection pooling.)
• Do not use group SAS authentication and use device SAS authentication instead (this is just good practice and a more secure approach)

Let us know if you find this useful. In the meanwhile, we will fix the issue and include it in the next release.