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

fix(iot-service): Fix Message disposal behavior #1629

Merged
merged 3 commits into from
Oct 30, 2020
Merged

Conversation

abhipsaMisra
Copy link
Member

@abhipsaMisra abhipsaMisra commented Oct 28, 2020

Our Message class on the service client contained a reference to the actual AmqpMessage which is sent when SentAsync() was called. Since the AmqpMessage is disposed after each Send attempt, resending the same IoT Hub message (on network failure etc) would result in us attempting to resend the same disposed Amqp message, resulting in an ObjectDisposedException.

This PR removes the AmqpMessage from being a property of IoT Hub message, resulting in us being able to manage their lifetimes independently.

Is is very surprising however that no one has reported this issue yet. With our current code, if you attempt to resend the same message instance multiple times, you will always get an ObjectDisposedException.

You can use the sample here to test this behavior (simulate network disconnect to have the sdk resend the same Message multiple times).

Even this fails:

private async Task SendSameMessage()
{
    var message = new Message(Encoding.UTF8.GetBytes("test"));

    for (int i = 0; i < 5; i++)
    {
        await _serviceClient.SendAsync(_deviceId, message);
        Console.WriteLine($"done {i}");
    }
}

Fix for #1638

@abhipsaMisra abhipsaMisra changed the base branch from azabbasi/AmqpServiceClientLogs to master October 29, 2020 00:41
@abhipsaMisra
Copy link
Member Author

We cannot combine the disposal of AmqpMessage with disposal of IoT Hub Message either, since an AmqpMessage sent (or attempted) over the link once cannot be resent.
So we need to dispose the AmqpMessage after every send attempt.

[Microsoft-Azure-Devices-Service-Client-ErrorMessage] (AmqpServiceClient#32106157, SendAsync, SendAsync threw an exception: System.InvalidOperationException: A message cannot be sent because it is either received from a link or is already sent over a link.
   at Microsoft.Azure.Amqp.SendingAmqpLink.BeginSendMessage(AmqpMessage message, ArraySegment`1 deliveryTag, ArraySegment`1 txnId, TimeSpan timeout, AsyncCallback callback, Object state)
   at Microsoft.Azure.Amqp.SendingAmqpLink.<>c__DisplayClass11_0.<SendMessageAsync>b__0(AsyncCallback c, Object s)
   at System.Threading.Tasks.TaskFactory`1.FromAsyncImpl(Func`3 beginMethod, Func`2 endFunction, Action`1 endAction, Object state, TaskCreationOptions creationOptions)
   at Microsoft.Azure.Amqp.SendingAmqpLink.SendMessageAsync(AmqpMessage message, ArraySegment`1 deliveryTag, ArraySegment`1 txnId, TimeSpan timeout)
   at Microsoft.Azure.Devices.AmqpServiceClient.SendAsync(String deviceId, Message message, Nullable`1 timeout) in C:\MyData\SourceCode\csharp\azure\iothub\service\src\AmqpServiceClient.cs:line 135).

@abhipsaMisra abhipsaMisra merged commit 7f8f868 into master Oct 30, 2020
@abhipsaMisra abhipsaMisra deleted the abmisr/c2dMessage branch October 30, 2020 00:07
timstewartm pushed a commit to timstewartm/azure-iot-sdk-csharp that referenced this pull request May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants