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

Investigate Force-Closing AMQP Links for Cancellation #19306

Closed
jsquire opened this issue Mar 6, 2021 · 3 comments · Fixed by #19955
Closed

Investigate Force-Closing AMQP Links for Cancellation #19306

jsquire opened this issue Mar 6, 2021 · 3 comments · Fixed by #19955
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. Event Hubs Service Bus
Milestone

Comments

@jsquire
Copy link
Member

jsquire commented Mar 6, 2021

Summary

The Event Hubs and Service Bus client libraries rely on the Azure AMQP library for communication with the messaging services. The AMQP library was built around the concept of timeouts and does not offer support for cancellation tokens. This leads to scenarios where the Event Hubs or Service Bus client libraries are awaiting an AMQP operation and are unable to respond to cancellation.

One of the scenarios where this causes pain for developers is when one of the messaging processors is in use. When a caller invokes the StopProcessingAsync or StopProcessing method, the processor will attempt to gracefully shut down, stopping tasks that are reading from the service and ensuring proper clean-up. Because the AMQP communication is timeout-based, when there are no messages flowing through the system, receive operations may block while being awaited for up to the configured TryTimeout, which by default is 60 seconds. This leads to a poor user experience where the processors take a long time to shut down and may cause issues with some host environments.

In a recent conversation with the AMQP library team, it was suggested that force-closing an active AMQP link could be safely used to cancel an operation in-flight. This would potentially allow the client types to respect cancellation while also gracefully shutting down and cleaning up. Doing so would introduce additional overhead for each service operation that would only provide a benefit in cancellation cases, but warrants investigation to understand the impact.

Scope of Work

  • Determine an approach to observing cancellation while awaiting a service operation. It may be possible to use the Azure.Core Task Extensions, or may take a similar form to the approach used during test scenarios:
public async Task<T> Execute<T>(Task<T> serviceTask, CancellationToken mainCancellation)
{
    using var linkedCancellation = CancellationTokenSource.CreateLinkedTokenSource(mainCancellation);   
    
    var delayTask = Task.Delay(Timeout.Infinite, linkedCancellation.Token);    
    
    var completedTask = await Task.WhenAny(
        delayTask,
        serviceTask
    ).ConfigureAwait(false);

    if (completedTask != delayTask)
    {
        linkedCancellation.Cancel();

        try
        {
            await delayTask.ConfigureAwait(false);
        }
        catch (OperationCanceledException)
        {
            // Expected
        }
        
        return (await serviceTask.ConfigureAwait(false));
    }
    
    await delayTask.ConfigureAwait(false);
    throw new TaskCanceledException("This should never execute");
}
  • Investigate the performance difference of applying an abstraction, including any additional allocations.

  • Assess the risk and potential for errors when refactoring.

  • Determine whether it makes sense to include a central executor for retry functionality in Event Hubs..

Success Criteria

  • Investigate the potential for use an approach to detecting cancellation and force-closing an AMQP link when cancelled.

  • Perform a small, tightly-scoped prototype and bench mark the difference to understand characteristics.

  • If the change is recommended, discuss with the .NET architects and the other .NET messaging developers to get buy-in.

  • If consensus is reached on the approach and need, the issues needed to track the implementation work have been created.

@jsquire jsquire added Service Bus Event Hubs Client This issue points to a problem in the data-plane of the library. labels Mar 6, 2021
@jsquire jsquire added this to the [2021] May milestone Mar 6, 2021
@jsquire jsquire self-assigned this Mar 6, 2021
@danielmarbach
Copy link
Contributor

I did a quick spike. What do you think about this approach?

#19888

@danielmarbach
Copy link
Contributor

Another one #19955

@jsquire
Copy link
Member Author

jsquire commented Apr 1, 2021

Reopening; we still don't yet have a final call on an approach.

@jsquire jsquire reopened this Apr 1, 2021
@jsquire jsquire modified the milestones: [2021] May, [2021] June Apr 5, 2021
@jsquire jsquire closed this as completed May 24, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. Event Hubs Service Bus
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants