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

ClientModel: Investigate feasibility of moving response buffering into the transport #41198

Closed
annelo-msft opened this issue Jan 12, 2024 · 2 comments · Fixed by #41772
Closed
Assignees
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. CoreDi 🚀 🔮 System.ClientModel Base Core library
Milestone

Comments

@annelo-msft
Copy link
Member

annelo-msft commented Jan 12, 2024

Investigate @KrzysztofCwalina's question regarding whether we can/should do buffering in the transport as opposed to doing it in a separate policy.

This issue tracks the request from this PR comment: #41085 (comment)

Investigation work-in-progress in this PR: #41651

@annelo-msft annelo-msft self-assigned this Jan 12, 2024
@annelo-msft annelo-msft added Azure.Core CoreDi 🚀 🔮 Client This issue points to a problem in the data-plane of the library. System.ClientModel Base Core library labels Jan 12, 2024
@annelo-msft annelo-msft added this to the 2024-02 milestone Jan 12, 2024
@github-actions github-actions bot added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Jan 12, 2024
@jsquire jsquire removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Jan 12, 2024
@annelo-msft
Copy link
Member Author

annelo-msft commented Jan 31, 2024

Based on #41651, it looks like this may be possible, but with the following risks:

  • Any direct calls to the public API HttpPipelineTransport.Process relying on the response.ContentStream to be un-buffered could fail, as this would now be buffered by default.
  • Any end-user custom policies inserted at HttpPipelinePosition.BeforeTransport relying on the transport not buffering the response content could fail.
  • Any mock transport implementations relying on being able to set response.ContentStream in the mock response and not have the transport replace the stream with a buffered stream could fail.
    • In the Azure.Core.Tests code-base we have several of these -- they work correctly when you explicitly opt-out of transport buffering by setting message.BufferResponse = false prior to calling transport.Process(message).
    • I still have an open question regarding whether tests like NonSeekableResponsesAreLimitedInLength fall in this category, or if it's possible this change could negatively impact logging.
  • It looks like no released Azure.Core-based clients would be impacted by this change, based on Core CI's passing in the checks for ClientModel Prototype: Move response buffering to transport #41651

@annelo-msft
Copy link
Member Author

Per Azure.Core v-team discussion and subsequent validations, this is not considered a breaking change. We will proceed with making this change, update Azure.Core tests that rely on message.BufferResponse = false to set this value accordingly, and not provide a fallback solution using quirking.

@github-actions github-actions bot locked and limited conversation to collaborators May 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. CoreDi 🚀 🔮 System.ClientModel Base Core library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants