ServiceBusRetryPolicy generic overloads to avoid closure capturing #19522

danielmarbach · 2021-03-15T15:36:06Z

Fixes #19126

All SDK Contribution checklist:

This checklist is used to make sure that common guidelines for a pull request are followed.

Please open PR in Draft mode if it is:
- Work in progress or not intended to be merged.
- Encountering multiple pipeline failures and working on fixes.
If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
I have read the contribution guidelines.
The pull request does not introduce breaking changes.

General Guidelines and Best Practices

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

Pull request includes test coverage for the included changes.

SDK Generation Guidelines

The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as the commitid of your swagger spec or link to the swagger spec, used to generate the code. (Track 2 only)
The *.csproj and AssemblyInfo.cs files have been updated with the new version of the SDK. Please double check nuget.org current release version.

Additional management plane SDK specific contribution checklist:

Note: Only applies to Microsoft.Azure.Management.[RP] or Azure.ResourceManager.[RP]

Include updated management metadata.
Update AzureRP.props to add/remove version info to maintain up to date API versions.

Management plane SDK Troubleshooting

If this is very first SDK for a services and you are adding new service folders directly under /SDK, please add new service label and/or contact assigned reviewer.
If the check fails at the Verify Code Generation step, please ensure:
- Do not modify any code in generated folders.
- Do not selectively include/remove generated files in the PR.
- Do use generate.ps1/cmd to generate this PR instead of calling autorest directly.
  Please pay attention to the @microsoft.csharp version output after running generate.ps1. If it is lower than current released version (2.3.82), please run it again as it should pull down the latest version,

Old outstanding PR cleanup

Please note:
If PRs (including draft) has been out for more than 60 days and there are no responses from our query or followups, they will be closed to maintain a concise list for our reviewers.

ghost · 2021-03-15T15:36:13Z

Thank you for your contribution @danielmarbach! We will review the pull request and get back to you soon.

danielmarbach · 2021-03-15T15:38:24Z

sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpSender.cs

@@ -224,12 +222,12 @@ internal class AmqpSender : TransportSender
        ///    Sends a set of messages to the associated Queue/Topic using a batched approach.
        /// </summary>
        ///
-        /// <param name="messageFactory"></param>
+        /// <param name="messages"></param>
        /// <param name="timeout"></param>
        /// <param name="cancellationToken">An optional <see cref="CancellationToken"/> instance to signal the request to cancel the operation.</param>
        ///
        internal virtual async Task SendBatchInternalAsync(


This is internal virtual, so I wasn't sure if I'm allowed to change it or not. Please advise

Yes feel free to change.

danielmarbach · 2021-03-15T15:39:28Z

sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpSender.cs

-                    cancellationToken).ConfigureAwait(false),
+                    token).ConfigureAwait(false),
+            this,
+            messageBatch.AsEnumerable<ServiceBusMessage>(),


I was fairly certain this is an OK change since the batch has a list internally. I couldn't see a reason why it would be required to cast to IEnumerable over and over again during retries. I preserved though the conversion to the AmqpMessage which seems to be the crucial part

sdk/servicebus/Azure.Messaging.ServiceBus/src/Primitives/ServiceBusRetryPolicy.cs

sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpSender.cs

sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpReceiver.cs

danielmarbach · 2021-03-15T15:55:42Z

sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpSender.cs

+                options,
+                _connectionScope,
+                cancellationToken);
+            return await createBatchTask.ConfigureAwait(false);
        }

        internal async ValueTask<TransportMessageBatch> CreateMessageBatchInternalAsync(


Example

Before

if (num1 != 0) { this.\u003C\u003E8__1 = new AmqpSender.\u003C\u003Ec__DisplayClass16_0(); this.\u003C\u003E8__1.\u003C\u003E4__this = this.\u003C\u003E4__this; this.\u003C\u003E8__1.options = this.options; this.\u003C\u003E8__1.messageBatch = (TransportMessageBatch) null; // ISSUE: method pointer configuredTaskAwaiter = amqpSender._retryPolicy.RunOperation(new Func<TimeSpan, Task>((object) this.\u003C\u003E8__1, __methodptr(\u003CCreateMessageBatchAsync\u003Eb__0)), (TransportConnectionScope) amqpSender._connectionScope, this.cancellationToken).ConfigureAwait(false).GetAwaiter(); if (!configuredTaskAwaiter.IsCompleted) { this.\u003C\u003E1__state = num2 = 0; this.\u003C\u003Eu__1 = configuredTaskAwaiter; // ISSUE: cast to a reference type // ISSUE: cast to a reference type ((AsyncValueTaskMethodBuilder<TransportMessageBatch>) ref this.\u003C\u003Et__builder).AwaitUnsafeOnCompleted<ConfiguredTaskAwaitable.ConfiguredTaskAwaiter, AmqpSender.\u003CCreateMessageBatchAsync\u003Ed__16>((M0&) ref configuredTaskAwaiter, (M1&) ref this); return; } }

After

if (num1 != 0) { // ISSUE: method pointer configuredTaskAwaiter = t1._retryPolicy.RunOperation<AmqpSender, CreateMessageBatchOptions, TransportMessageBatch>(AmqpSender.\u003C\u003Ec.\u003C\u003E9__16_0 ?? (AmqpSender.\u003C\u003Ec.\u003C\u003E9__16_0 = new Func<AmqpSender, CreateMessageBatchOptions, TimeSpan, CancellationToken, Task<TransportMessageBatch>>((object) AmqpSender.\u003C\u003Ec.\u003C\u003E9, __methodptr(\u003CCreateMessageBatchAsync\u003Eb__16_0))), t1, this.options, (TransportConnectionScope) t1._connectionScope, this.cancellationToken).ConfigureAwait(false).GetAwaiter(); if (!configuredTaskAwaiter.IsCompleted) { this.\u003C\u003E1__state = num2 = 0; this.\u003C\u003Eu__1 = configuredTaskAwaiter; // ISSUE: cast to a reference type // ISSUE: cast to a reference type ((AsyncValueTaskMethodBuilder<TransportMessageBatch>) ref this.\u003C\u003Et__builder).AwaitUnsafeOnCompleted<ConfiguredTaskAwaitable<TransportMessageBatch>.ConfiguredTaskAwaiter, AmqpSender.\u003CCreateMessageBatchAsync\u003Ed__16>((M0&) ref configuredTaskAwaiter, (M1&) ref this); return; } }

So we are saving two allocations now for every request and this on every callsite touched by this PR.

danielmarbach · 2021-03-19T13:37:17Z

sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpSender.cs

-            },
-            _connectionScope,
-            cancellationToken).ConfigureAwait(false);
-            return seqNumbers ?? Array.Empty<long>();


Can this ever be null? I tried to do in the ScheduleMessageInternalAsync return sequenceNumbers ?? Array.Empty<long>() and flow analysis claimed it cannot be null so I wonder if this is really needed

I don't think it can be null.

sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpSender.cs

danielmarbach · 2021-03-19T14:00:57Z

Getting now a bunch of error SA1141: Use tuple syntax. Will check

sdk/servicebus/Azure.Messaging.ServiceBus/src/Primitives/VoidResult.cs

This reverts commit 0420017.

danielmarbach · 2021-03-19T16:01:21Z

Hope this time the build will not be canceled. It looks though like master is also in a weird state

danielmarbach · 2021-03-19T16:24:55Z

Would you like me to squash things a bit to clean up the history or ok as is?

JoshLove-msft · 2021-03-19T18:21:34Z

/azp run net - servicebus - tests

azure-pipelines · 2021-03-19T18:21:45Z

Azure Pipelines successfully started running 1 pipeline(s).

JoshLove-msft · 2021-03-19T18:21:52Z

Would you like me to squash things a bit to clean up the history or ok as is?

No need it will get squashed when merging.

JoshLove-msft · 2021-03-19T18:22:15Z

/azp run net - servicebus - ci

azure-pipelines · 2021-03-19T18:22:28Z

Azure Pipelines successfully started running 1 pipeline(s).

jsquire

Thank you, @danielmarbach, for identifying the opportunity here and for making the change. The current form looks good to me structurally. I have some concerns around some of the naming - I find the abbreviations make it harder to read and understand context at a glance. I'd really appreciate if we could err on the side of more expressive names.

I'm going to defer the call on final approval to @JoshLove-msft.

sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpSender.cs

jsquire · 2021-03-19T20:12:44Z

sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpSender.cs

-            return messageBatch;
+            return await _retryPolicy.RunOperation(static async (value, timeout, _) =>
+                {
+                    var (sender, ops) = value;


nit: It's not clear to me what ops is without looking at the original context. Can we go with options again?

Same comment as abobeu

sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpSender.cs

sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpReceiver.cs

sdk/servicebus/Azure.Messaging.ServiceBus/src/Primitives/ServiceBusRetryPolicy.cs

JoshLove-msft · 2021-03-22T01:06:50Z

/azp run net - servicebus - tests

azure-pipelines · 2021-03-22T01:07:00Z

Azure Pipelines successfully started running 1 pipeline(s).

danielmarbach added 3 commits March 15, 2021 16:17

ServiceBusRetryPolicy generic overloads to avoid closure capturing

ab3039e

Avoid closure on sending and retrying batched messages

24f3757

Do conversion once

61a7637

ghost added Service Bus customer-reported Issues that are reported by GitHub users external to the Azure organization. labels Mar 15, 2021

ghost added the Community Contribution Community members are working on the issue label Mar 15, 2021

danielmarbach commented Mar 15, 2021

View reviewed changes

sdk/servicebus/Azure.Messaging.ServiceBus/src/Primitives/ServiceBusRetryPolicy.cs Outdated Show resolved Hide resolved

danielmarbach commented Mar 15, 2021

View reviewed changes

sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpSender.cs Outdated Show resolved Hide resolved

danielmarbach commented Mar 15, 2021

View reviewed changes

sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpReceiver.cs Show resolved Hide resolved

pakrym reviewed Mar 15, 2021

View reviewed changes

sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpReceiver.cs Outdated Show resolved Hide resolved

danielmarbach commented Mar 15, 2021

View reviewed changes

danielmarbach mentioned this pull request Mar 15, 2021

[QUERY] ServiceBusRetryPolicy anonymous class allocation #19126

Closed

danielmarbach added 2 commits March 15, 2021 17:24

Address leftover RunOperation in AmqpSender

6bbd1a7

Static lambdas

6198b0a

danielmarbach marked this pull request as ready for review March 19, 2021 13:35

danielmarbach requested review from JoshLove-msft and jsquire as code owners March 19, 2021 13:35

danielmarbach commented Mar 19, 2021

View reviewed changes

sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpSender.cs Show resolved Hide resolved

danielmarbach added 2 commits March 19, 2021 15:39

Switch to value tuple approach

54874f0

Remove async/await where not needed

0420017

danielmarbach force-pushed the retry-policy-allocations branch from 458d86b to 0420017 Compare March 19, 2021 14:40

pakrym reviewed Mar 19, 2021

View reviewed changes

sdk/servicebus/Azure.Messaging.ServiceBus/src/Primitives/VoidResult.cs Outdated Show resolved Hide resolved

danielmarbach added 3 commits March 19, 2021 16:32

Cleanup VoidResult usage

ec822be

Revert "Remove async/await where not needed"

2a45070

This reverts commit 0420017.

Switch to ValueTask

d2104f1

jsquire reviewed Mar 19, 2021

View reviewed changes

danielmarbach added 3 commits March 21, 2021 14:50

Use more intention revealing names in static lambdas

6993c08

Remove unnecessary null check

870893d

Return expected public type earlier

344ca21

JoshLove-msft approved these changes Mar 22, 2021

View reviewed changes

JoshLove-msft merged commit e3eca79 into Azure:master Mar 22, 2021

danielmarbach deleted the retry-policy-allocations branch May 5, 2021 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ServiceBusRetryPolicy generic overloads to avoid closure capturing #19522

ServiceBusRetryPolicy generic overloads to avoid closure capturing #19522

danielmarbach commented Mar 15, 2021 •

edited

Loading

ghost commented Mar 15, 2021

danielmarbach Mar 15, 2021

JoshLove-msft Mar 19, 2021

danielmarbach Mar 15, 2021

danielmarbach Mar 15, 2021 •

edited

Loading

danielmarbach Mar 19, 2021

JoshLove-msft Mar 19, 2021

danielmarbach commented Mar 19, 2021 •

edited

Loading

danielmarbach commented Mar 19, 2021

danielmarbach commented Mar 19, 2021

JoshLove-msft commented Mar 19, 2021

azure-pipelines bot commented Mar 19, 2021

JoshLove-msft commented Mar 19, 2021

JoshLove-msft commented Mar 19, 2021

azure-pipelines bot commented Mar 19, 2021

jsquire left a comment

jsquire Mar 19, 2021

danielmarbach Mar 20, 2021

JoshLove-msft commented Mar 22, 2021

azure-pipelines bot commented Mar 22, 2021

ServiceBusRetryPolicy generic overloads to avoid closure capturing #19522

ServiceBusRetryPolicy generic overloads to avoid closure capturing #19522

Conversation

danielmarbach commented Mar 15, 2021 • edited Loading

All SDK Contribution checklist:

Additional management plane SDK specific contribution checklist:

Management plane SDK Troubleshooting

Old outstanding PR cleanup

ghost commented Mar 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielmarbach Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

Before

After

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielmarbach commented Mar 19, 2021 • edited Loading

danielmarbach commented Mar 19, 2021

danielmarbach commented Mar 19, 2021

JoshLove-msft commented Mar 19, 2021

azure-pipelines bot commented Mar 19, 2021

JoshLove-msft commented Mar 19, 2021

JoshLove-msft commented Mar 19, 2021

azure-pipelines bot commented Mar 19, 2021

jsquire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshLove-msft commented Mar 22, 2021

azure-pipelines bot commented Mar 22, 2021

danielmarbach commented Mar 15, 2021 •

edited

Loading

danielmarbach Mar 15, 2021 •

edited

Loading

danielmarbach commented Mar 19, 2021 •

edited

Loading