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

SocketsHttpHandler: Support zero-byte read on response streams #61475

Closed
geoffkizer opened this issue Nov 11, 2021 · 3 comments · Fixed by #61913
Closed

SocketsHttpHandler: Support zero-byte read on response streams #61475

geoffkizer opened this issue Nov 11, 2021 · 3 comments · Fixed by #61913
Assignees
Labels
area-System.Net.Http enhancement Product code improvement that does NOT require public API changes/additions tenet-performance Performance related issue
Milestone

Comments

@geoffkizer
Copy link
Contributor

Supporting zero-byte read helps users of these APIs reduce memory usage. It allows them to release their read buffers when no data is currently available.

We should support this in our HTTP response streams in SocketsHttpHandler, including RawConnectionStream since this is used for WebSockets and tunneling scenarios.

Ideally we would also release our own read buffer (if any and if possible) and issue a zero-byte read against the underlying connection. This is what SslStream does (as of 6.0). The idea here is that the user has indicated their interest in minimizing buffer memory usage, and so we should do the same for our own internal buffers.

I split this issue off from #61223.

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Net.Http untriaged New issue has not been triaged by the area owner labels Nov 11, 2021
@ghost
Copy link

ghost commented Nov 11, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Supporting zero-byte read helps users of these APIs reduce memory usage. It allows them to release their read buffers when no data is currently available.

We should support this in our HTTP response streams in SocketsHttpHandler, including RawConnectionStream since this is used for WebSockets and tunneling scenarios.

Ideally we would also release our own read buffer (if any and if possible) and issue a zero-byte read against the underlying connection. This is what SslStream does (as of 6.0). The idea here is that the user has indicated their interest in minimizing buffer memory usage, and so we should do the same for our own internal buffers.

I split this issue off from #61223.

Author: geoffkizer
Assignees: -
Labels:

area-System.Net.Http, untriaged

Milestone: -

@karelz karelz added this to the 7.0.0 milestone Nov 16, 2021
@karelz karelz added enhancement Product code improvement that does NOT require public API changes/additions tenet-performance Performance related issue and removed untriaged New issue has not been triaged by the area owner labels Nov 16, 2021
@karelz
Copy link
Member

karelz commented Nov 16, 2021

Triage: We have multiple customers who might benefit from this (savings in COGS) -- gRPC and 2 partner teams.

We might want to double check if SslStream 0-byte reads do not allocate. It might not be 100% working there. We do not have perf coverage here.

@MihaZupan MihaZupan self-assigned this Nov 16, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 22, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Nov 29, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Dec 30, 2021
@karelz
Copy link
Member

karelz commented Aug 12, 2022

Partial fix is also in 6.0.9 (see PR #72804)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http enhancement Product code improvement that does NOT require public API changes/additions tenet-performance Performance related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants