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

[FEATURE REQ] Azure.Messaging.ServiceBus_7.0.0 - Add maxWaitTime parameter to ReceiveMessagesAsync #17581

Closed
cosminstirbu opened this issue Dec 16, 2020 · 7 comments
Assignees
Labels
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. feature-request This issue requires a new behavior in the product in order be resolved. Service Bus
Milestone

Comments

@cosminstirbu
Copy link

Library or service name.
Azure.Messaging.ServiceBus_7.0.0

Is your feature request related to a problem? Please describe.

We are using Azure.Messaging.ServiceBus to write integration tests that check the rules we have set on our subscriptions.

Some of these tests are checking that unintended messages are not received by a subscription.

To do this we're using public virtual IAsyncEnumerable<ServiceBusReceivedMessage> ReceiveMessagesAsync([EnumeratorCancellation] CancellationToken cancellationToken = default) by passing a CancellationToken that uses a TimeStamp.

However it seems that the CancellationToken is not enough, because the method calls under the hood public virtual Task<ServiceBusReceivedMessage> ReceiveMessageAsync(TimeSpan? maxWaitTime = null, CancellationToken cancellationToken = default); without passing any maxWaitTime.

This means that even if we want to use the CancellationToken to wait at most 5 seconds for a message to not be received, it will actually wait 1 minute (the default value of maxWaitTime).

Our current workaround was to create an extension that takes the maxWaitTime parameter as well, however it would be nice to have this part of the SDK.

This is our extension:

        // Existing method doesn't allow passing maxWaitTime
        // https://github.com/Azure/azure-sdk-for-net/blob/6adff9084cf51a0f082e813ee8d7b7daef117642/sdk/servicebus/Azure.Messaging.ServiceBus/src/Receiver/ServiceBusReceiver.cs#L290
        public static async IAsyncEnumerable<ServiceBusReceivedMessage> ReceiveMessagesAsync(this ServiceBusReceiver self, TimeSpan? maxWaitTime = null, [EnumeratorCancellation] CancellationToken cancellationToken = default)
        {
            while (!cancellationToken.IsCancellationRequested)
            {
                var msg = await self.ReceiveMessageAsync(maxWaitTime, cancellationToken).ConfigureAwait(false);

                if (msg == null)
                {
                    continue;
                }

                yield return msg;
            }

            // Surface the TCE to ensure deterministic behavior when cancelling.
            cancellationToken.ThrowIfCancellationRequested();
        }
@ghost ghost added needs-triage Workflow: 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 Dec 16, 2020
@jsquire jsquire added Client This issue points to a problem in the data-plane of the library. needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team Service Bus labels Dec 16, 2020
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Dec 16, 2020
@jsquire
Copy link
Member

jsquire commented Dec 16, 2020

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

@JoshLove-msft
Copy link
Member

The maxWaitTime parameter wasn't included on the IAsyncEnumerable because this method is most useful when you want to keep receiving messages indefinitely without needing the finer level of control that the ReceiveMessageAsync or other ReceiveMessagesAsync overload offer. Would either of those work for your scenario?

@cosminstirbu
Copy link
Author

cosminstirbu commented Dec 17, 2020

Based on our use case ("trigger" a message that doesn't match the rules on the subscription and make sure that in a given period of time it isn't received) the IAsyncEnumerable method is the best candidate for us.

We can't use the other ones because we also want to handle the scenarios when unrelated messages (from other manual or automated testing going on at the time of running the test) might be received as well. This means that we continue to receive messages and filter them based on a test controlled customId as per the example below:

        public static async Task<ServiceBusReceivedMessage> ReceiveMessageAsync(this ServiceBusReceiver self, string customId, TimeSpan maxWaitTime)
        {
            var cancellationToken = new CancellationTokenSource(maxWaitTime).Token;

            await foreach (var message in self.ReceiveMessagesAsync(cancellationToken))
            {
                if (message.ApplicationProperties["customId"].Equals(customId))
                {
                    return message;
                }
            }

            // This is never reached because of the exception caused by maxWaitTime
            return null!;
        }

And this is how we use it:

            Func<Task> receiveFunc = async () => await serviceBusReceiver.ReceiveMessageAsync("someTestControledId", DefaultDontReceiveMaxWaitTime);
            receiveFunc.Should().Throw<TaskCanceledException>();

I agree that our use case is not really common, so I understand if the suggested feature doesn't make sense from a broader perspective.

However, as mentioned initially, if we use a CancellationToken instantiated with a TimeSpan shorter than the default maxWaitTime then that CancellationToken won't trigger any timeout.

I understand that the intended use case is not to pass CancellationToken instantiated with a TimeSpan, so I guess it makes sense to keep it as is.

@JoshLove-msft
Copy link
Member

Thanks for the additional context. I think we can leave this issue open for now to track this feature request.

@jsquire jsquire added feature-request This issue requires a new behavior in the product in order 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 Dec 30, 2020
@jsquire jsquire added this to the Backlog milestone Dec 30, 2020
@jsquire jsquire removed the needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team label Dec 30, 2020
@ghost ghost added the no-recent-activity There has been no recent activity on this issue. label Mar 12, 2021
@ghost
Copy link

ghost commented Mar 12, 2021

Hi @cosminstirbu. Thank you, for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

@ghost ghost removed the no-recent-activity There has been no recent activity on this issue. label Mar 13, 2021
@ghost
Copy link

ghost commented Mar 13, 2021

Hi @cosminstirbu. There was a mistake and this issue was unintentionally flagged as a stale pull request. The label has been removed and the issue will remain active; no action is needed on your part. Apologies for the inconvenience.

@JoshLove-msft
Copy link
Member

Closing this out for now as we have not seen any feedback indicating that this would be useful outside of the testing use case presented here.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 28, 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. customer-reported Issues that are reported by GitHub users external to the Azure organization. feature-request This issue requires a new behavior in the product in order be resolved. Service Bus
Projects
None yet
Development

No branches or pull requests

3 participants