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

Support streaming calls in the SubchannelCallTracker #2133

Open
Khazuar opened this issue May 17, 2023 · 5 comments
Open

Support streaming calls in the SubchannelCallTracker #2133

Khazuar opened this issue May 17, 2023 · 5 comments
Labels
bug Something isn't working
Milestone

Comments

@Khazuar
Copy link

Khazuar commented May 17, 2023

What version of gRPC and what language are you using?

Grpc.AspNetCore v2.53.0 in C#

What operating system (Linux, Windows,...) and version?

Mac OSX Ventura

What runtime / compiler are you using (e.g. .NET Core SDK version dotnet --info)

.NET 7.0.200

What did you do?

I wrote a custom load balancer for client side load balancing. I used the ISubchannelCallTracker interface to detect when calls happen and complete on a subchannel to know what load they are bearing. This works fine for non-streaming grpc-calls, but it breaks down for streaming-calls.

What did you expect to see?

That the Complete-function of the ISubchannelCallTracker is called once the AsyncServerStreamingCall is disposed in the user-code.

What did you see instead?

The Complete-function is called right after the grpc-call is made, before even the first item is retriebed from the ResponseStream via MoveNext.

Thoughts

I saw this comment in the source and wondered what it would take to handle this case correctly. AsyncServerStreamingCall does have a disposeAction already, but its not clear how it relates to the HttpResponseMessage in the other place and what else needs to be thought of. I would be happy to provide a PR if I could get some pointers.

@Khazuar Khazuar added the bug Something isn't working label May 17, 2023
@JamesNK
Copy link
Member

JamesNK commented May 17, 2023

You're right that the current behavior today isn't great for streaming. It should be improved.

I don't know the right way to implement this. I need to research. Because this layer works on HTTP requests, I don't think it knows about gRPC objects and events.

When I wrote this code, I imagined that it would require wrapping the response HttpContent and listening for a dispose or read end event.

@karelz Could you recommend someone who would know the best way to listen for the end of the overall request, i.e. HTTP response stream end. Do you know of other libraries doing something like this with HttpClient today?

@MihaZupan
Copy link

MihaZupan commented May 18, 2023

the best way to listen for the end of the overall request, i.e. HTTP response stream end

There's no "nice" way to do that currently.

I think your options are either:

  1. Create and return your own HttpResponseMessage type where you copy over all the properties and hook into the dispose pattern
  2. Replace the Content property with your own that returns a custom stream that tracks EOF/dispose.

Both have a cost (e.g. reallocating the header collection). If these are long-lived responses, option 1 is likely cheaper.

Depending on what's more appropriate for your scenario, note that stream disposal/EOF may happen earlier than response disposal.


I don't know of good examples of libraries doing this, HttpClient itself does it internally but we have full control over the content objects.

@Khazuar
Copy link
Author

Khazuar commented Jun 1, 2023

@JamesNK I'd appreciate if a quick solution for this were to be found. The library is just behaving incorrectly at the moment, and there don't seem to be many users of the feature-combination "custom loadbalancer that tracks calls" + "streaming calls", or this bug would have been found earlier. Therefore, a few allocations more will not impact many people. IMHO solving this "properly" via changes to dotnet/runtime might be a performance-improvement to be done later on.

@JamesNK
Copy link
Member

JamesNK commented Jun 1, 2023

Thank you for creating a PR.

I need to test this thoroughly and write unit tests. I'm sure it works for calls that successfully read to the end of the request. This is what you probably tested. But I want to verify calls are correctly ended for failure conditions. Does it still work if the gRPC call ends because of cancellation? Or deadline. Or server abort. Or someone doesn't read from the stream, and it times out. And so on.

@JamesNK JamesNK added this to the Backlog milestone Jun 15, 2023
@JamesNK
Copy link
Member

JamesNK commented Jun 15, 2023

I don't believe this is possible without changes to HttpClient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants