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

[LLC] Remove exception allocation from generated "Exists" method #1531

Open
annelo-msft opened this issue Sep 24, 2021 · 4 comments
Open

[LLC] Remove exception allocation from generated "Exists" method #1531

annelo-msft opened this issue Sep 24, 2021 · 4 comments
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. DPG v3 Version 3 of AutoRest C# generator. WS: Code Generation

Comments

@annelo-msft
Copy link
Member

annelo-msft commented Sep 24, 2021

To resolve this issue, we're pre-allocating an exception in the default: block of this switch statement.

Once this issue has been completed, we will have all the pieces we need in place to wait to allocate the exception until response.Value is called, i.e. we believe we will have a public type called OptionalResponse<T>, and in its implementation of the Value property, it can use the new constructor on RequestFailedException to create the exception.

Depends on completion of: Azure/azure-sdk-for-net#23372

@annelo-msft annelo-msft added the v3 Version 3 of AutoRest C# generator. label Sep 24, 2021
@annelo-msft annelo-msft added Client This issue points to a problem in the data-plane of the library. DPG labels Oct 21, 2021
@annelo-msft
Copy link
Member Author

annelo-msft commented Dec 3, 2021

Update: OptionalResponse<T> was implemented as ErrorResponse<T>

Per discussion regarding calling the sync constructor where we're currently calling an async method:

[Yesterday 10:12 AM] @pakrym
Most of the time the response is buffered and it's OK to use the sync version. There are some requests that we don't buffer and for them calling CreateRequestFailedException will result in a sync read from a network connection.

Another potential issue here is that HttpMessage is disposed at the end of the service method call, and so does the response. So you need to check if it is possible to extract exception using CreateRequestFailedException after HttpMessage is disposed.

Same problem here. For buffered responses content will still be available. For non-buffered content will be a disposed network stream.
[Yesterday 10:12 AM] @pakrym
I wonder if we need to add Response.BufferContentAsync() or just a feature that always buffers error responses as part of the pipeline
It will make the error handling story way simpler if we can presume that all failed responses have a buffered content.

Let's add the BufferContent() and BufferContentAsync() to Response, but wait to do this performance optimization.

@annelo-msft annelo-msft added the blocking-release Blocks release label Mar 29, 2022
@lirenhe lirenhe assigned chunyu3 and unassigned fengzhou-msft Mar 31, 2022
@chunyu3
Copy link
Member

chunyu3 commented Apr 22, 2022

Hi @annelo-msft What's purpose of this issue? Is it to replace the current exception-generate method (clientDiagnostics.CreateRequestFailedException) to use RequestFailedException method. see following:
Current:
image

update to:
image

@annelo-msft
Copy link
Member Author

Since this method is never used, let's not block GA on it.

@annelo-msft annelo-msft removed the blocking-release Blocks release label Apr 22, 2022
@chunyu3
Copy link
Member

chunyu3 commented Apr 24, 2022

@annelo-msft Is the issue to replace the exception-generate method as above comment? Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. DPG v3 Version 3 of AutoRest C# generator. WS: Code Generation
Projects
None yet
Development

No branches or pull requests

5 participants