[LLC] For Discussion: Add constructor that takes Response to RequestFailedException #24248

annelo-msft · 2021-09-27T15:18:13Z

Opening this PR as draft, so we can have a discussion about the points raised in the implementation of the new constructor for RequestFailedException.

Our overarching goal (per #23372) is to implement this API:

        public static implicit operator Pet(Response response)
        {
            if (response.IsError())
            {
                throw new RequestFailedException(response);
            }

            return DeserializePet(JsonDocument.Parse(response.Content.ToMemory()));
        }

annelo-msft · 2021-09-27T15:19:54Z

sdk/core/Azure.Core.Experimental/tests/LowLevelClient/PetStoreClient.cs

+                        // ??
+                        // (that would be so much tidier!)
+                        // QUESTION: What happens in the Async case, do we still need it?
+                        // Because we can't have an async constructor.


Open question here on what we would do in async APIs if we wanted to replace the _clientDiagnostics call with new RequestFailedException(response) instead.

Per @pakrym:
It's not a huge problem
most client calls are buffered
so all reads are sync
but for the last 1% I wonder if we should auto buffer error responses

annelo-msft · 2021-09-27T15:20:48Z

sdk/core/Azure.Core.Experimental/tests/LowLevelClientTests.cs

+		//    _clientDiagnostics.CreateRFException() -- this happens via different constructors (Note: does it have to?  Could we refactor this?)
+		// 
+		// 2) In the ResponseStatusOptions.NoThrow case, we're going through code paths in
+		//    ResponseClassifier, which will become the only path after resolution of #24031


Note: we'll reconcile these two code paths in #24031

annelo-msft · 2021-09-27T15:21:26Z

sdk/core/Azure.Core.Experimental/tests/LowLevelClientTests.cs

+            {
+				// NOTE: is it weird that we're saying NoThrow here and it throws?
+				// This looks confusing to me as someone reading this code.
+                Pet pet = await client.GetPetAsync("pet1", ResponseStatusOption.NoThrow);


Question: do we like the look of this API, with the exception thrown when we pass NoThrow?

annelo-msft · 2021-09-27T15:21:53Z

sdk/core/Azure.Core/src/Pipeline/HttpPipeline.cs

-            return _pipeline.Span[0].ProcessAsync(message, _pipeline.Slice(1));
+
+			// TODO: We need to discuss possible performance implications of this
+            await _pipeline.Span[0].ProcessAsync(message, _pipeline.Slice(1)).ConfigureAwait(false);


Question: what are the performance implications of making this change -- it would affect all libraries.

I would propose that we don't do this change now. After LLC RC, we can measure the impact and either do this change or leave the code as-is.

That makes sense. Given that this approach relies on Response.RequestClassifier being internal, the approach to putting this functionality in Azure.Core.Experimental may end up looking quite different. I can do that implementation, but I wanted to make sure we discussed what it would look like in Core in the long term so we didn't end up with unpleasant surprises when we made that transition.

annelo-msft · 2021-09-27T15:23:21Z

sdk/core/Azure.Core/src/ResponseClassifier.cs

@@ -2,15 +2,51 @@
 // Licensed under the MIT License.

 using System;
+using System.Collections.Generic;


Note, only some of the ClientDiagnostics functionality has been moved over to ResponseClassifier here; this will be addressed in #24031.

annelo-msft · 2021-09-27T15:25:41Z

sdk/core/Azure.Core/src/ResponseClassifier.cs

+
+            // Note: we can ignore additionalInfo for now (for the purposes of the discussion of this design)
+            // because it's only passed in by exception creation methods called by the service libraries,
+            // and isn't useful when we are creating the exception from the response directly.


This points to an opportunity to make some changes to the ClientDiagnostics API when we make it public in ResponseClassifier. We can clarify what's strictly required (i.e. the response), and what available input properties are there to allow service libraries to customize the exception.

KrzysztofCwalina · 2021-09-27T15:43:34Z

sdk/core/Azure.Core/src/Response.cs

@@ -144,5 +144,8 @@ internal static void DisposeStreamIfNotBuffered(ref Stream? stream)
                stream = null;
            }
        }
+
+		// TODO: Are we happy with this internal property on Response?


It's reasonable, but why would we not add a property pointing to the whole message, i.e. the classifier is already in the message.

Classifier is the only thing that survives the message lifetime.

My concern about adding the whole message is that it contains Request which may be disposed before the Response lifetime is over, so we would have to take extra care in Response to only access parts of message at the right time. This approach side-steps that issue.

But if we are careful we can reference the entire thing.

But if we are careful we can reference the entire thing.

Do you mean that we can include message as an internal property as long as we make sure we don't access Request after it is disposed? Just out of curiosity, what is the benefit of having the whole message over just the ResponseClassifier?

Do you mean that we can include message as an internal property as long as we make sure we don't access Request after it is disposed?

Yes

Just out of curiosity, what is the benefit of having the whole message over just the ResponseClassifier?

Next time we need something from the message we don't need to add another property. But it's a pretty weak reason for me.

KrzysztofCwalina · 2021-09-27T15:51:26Z

sdk/core/Azure.Core/src/Pipeline/HttpPipeline.cs

-            return _pipeline.Span[0].ProcessAsync(message, _pipeline.Slice(1));
+
+			// TODO: We need to discuss possible performance implications of this
+            await _pipeline.Span[0].ProcessAsync(message, _pipeline.Slice(1)).ConfigureAwait(false);


I would propose that we don't do this change now. After LLC RC, we can measure the impact and either do this change or leave the code as-is.

pakrym · 2021-09-27T15:59:44Z

sdk/core/Azure.Core/src/ResponseClassifier.cs

+        /// Initializes a new instance of <see cref="ResponseClassifier"/>.
+        /// </summary>
+        /// <param name="options"></param>
+        public ResponseClassifier(ClientOptions options)


This is interesting. We will need to enforce everyone using this ctor and ban the parameterless one.

I don't know that they always can -- sometimes ResponseClassifier is newed in Core and would take DefaultClientOptions then. We should think this through and see if there's another way.

I don't know that they always can

We have to. Otherwise, exception formatting would sanitize all the headers even if they were marked as safe by the client author.

annelo-msft · 2021-09-27T18:15:23Z

Had the discussion I was interested in, closing this in favor of an approach that sets the Reponse property in Core.Experimental.

Fix sqlvm.json (Azure#24248)

annelo-msft added 3 commits September 24, 2021 15:43

use the Response.IsError property in an E2E test

fc0866a

add ResponsePropertiesPolicy into pipeline in generated code.

29af7d7

Trial implementation for discussion

0934a8f

ghost added the Azure.Core label Sep 27, 2021

annelo-msft requested review from KrzysztofCwalina, ellismg, tg-msft and pakrym September 27, 2021 15:18

annelo-msft commented Sep 27, 2021

View reviewed changes

KrzysztofCwalina reviewed Sep 27, 2021

View reviewed changes

annelo-msft added 2 commits September 27, 2021 08:56

Merge remote-tracking branch 'upstream/main' into llc-rfe-constructor

afddd07

update generated APIs and some nits

e29fbfd

pakrym reviewed Sep 27, 2021

View reviewed changes

annelo-msft closed this Sep 27, 2021

annelo-msft requested review from ShivangiReja and AlexanderSher September 28, 2021 17:22

azure-sdk pushed a commit to azure-sdk/azure-sdk-for-net that referenced this pull request Jun 2, 2023

CodeGen from PR 24248 in Azure/azure-rest-api-specs

87c4da8

Fix sqlvm.json (Azure#24248)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LLC] For Discussion: Add constructor that takes Response to RequestFailedException #24248

[LLC] For Discussion: Add constructor that takes Response to RequestFailedException #24248

annelo-msft commented Sep 27, 2021

annelo-msft Sep 27, 2021

annelo-msft Sep 28, 2021

annelo-msft Sep 27, 2021

annelo-msft Sep 27, 2021

annelo-msft Sep 27, 2021 •

edited

Loading

KrzysztofCwalina Sep 27, 2021

annelo-msft Sep 27, 2021

annelo-msft Sep 27, 2021

annelo-msft Sep 27, 2021

KrzysztofCwalina Sep 27, 2021

pakrym Sep 27, 2021

annelo-msft Sep 27, 2021

pakrym Sep 27, 2021

annelo-msft Sep 27, 2021

pakrym Sep 27, 2021

KrzysztofCwalina Sep 27, 2021

pakrym Sep 27, 2021

annelo-msft Sep 27, 2021

pakrym Sep 27, 2021

annelo-msft Sep 27, 2021

annelo-msft commented Sep 27, 2021

[LLC] For Discussion: Add constructor that takes Response to RequestFailedException #24248

[LLC] For Discussion: Add constructor that takes Response to RequestFailedException #24248

Conversation

annelo-msft commented Sep 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annelo-msft Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annelo-msft commented Sep 27, 2021

annelo-msft Sep 27, 2021 •

edited

Loading