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

NullableResponse<T> should implement IDisposable #35565

Closed
heaths opened this issue Apr 13, 2023 · 1 comment
Closed

NullableResponse<T> should implement IDisposable #35565

heaths opened this issue Apr 13, 2023 · 1 comment
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library.

Comments

@heaths
Copy link
Member

heaths commented Apr 13, 2023

Because ValueResponse<T> takes a Response as a parameter and holds onto it, and because Response implements IDisposable, ValueResponse<T> should dispose the Response because it's effectively taking ownership of it. However, because ValueResponse<T> is internal and we could possibly expose a NullableResponse<T> in a public API (see #33767 for context), we should implement IDisposable on NullableResponse<T>. It'd be a virtual no-op but overridden in ValueResponse<T>.

@heaths heaths added Client This issue points to a problem in the data-plane of the library. Azure.Core labels Apr 13, 2023
@annelo-msft
Copy link
Member

Making NullableResponse<T> IDisposable would make Response<T> IDisposable and require usage patterns for all clients to change to dispose the return values of all service method calls. Given how broadly this would impact users, I don't think we would want to implement this in Azure.Core. Service methods are responsible for disposing the response (HttpMessage should be in a using block in service method implementations), so Response is disposed when the service method returns, while remaining usable for end-user purposes. In cases where resources that need to be disposed (such as network streams) are returned to the end user, convenience methods should be extracting those values from the message and response and returning types like Response<Stream> while making it clear in the refdocs that the end-user needs to dispose these values.

@heaths, if there's a specific case you're concerned about in a specific client, I'm happy to reopen and find the right solution for that case.

@github-actions github-actions bot locked and limited conversation to collaborators Aug 18, 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.
Projects
None yet
Development

No branches or pull requests

2 participants