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

Call Complete on SubchannelCallTracker after HttpContent has been disposed #2148

Closed

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Jun 2, 2023

Update on #2139

  • Adds unit tests.
  • Fixes scenario a streaming call is never used (the response stream isn't read from and the call isn't disposed) and the server aborts the call.

FYI @Khazuar

@JamesNK
Copy link
Member Author

JamesNK commented Jun 4, 2023

I looked at this more, and unfortunately, I don't think it is possible to accurately track when the response ends in all situations in .NET.

What does work is getting notifications when:

  1. The client aborts the response.
  2. The client reads to the end of the response.

The problem is there isn't a way to get a notification from the server. For example, there isn't a way to get a notification that the server has aborted the request if the client abandons using it, such as never disposes the response or stops reading the response stream before the end.

The only way I can see how to get that information is if the client has a pending read call on the response stream. A way to get the notification could always be to have a Stream.ReadAsync pending on the real response stream. If it succeeds, write the data to a buffer from which the client reads with another stream. If it fails, then the response is in a bad state, so report it is finished. However, this idea doesn't work if the server constantly sends data and the client isn't reading it. The buffer grows indefinitely.

@Khazuar
Copy link

Khazuar commented Jun 4, 2023

Mhh .. that's suboptimal. I'd have argued that clients are supposed to call Dispose on the response whenever they are done with it, including the streamed data. As far as I understand that would cover server-side aborts as well, correct? I do understand though, that this is not super obvious to users.

@JamesNK JamesNK closed this Jun 6, 2023
@Khazuar
Copy link

Khazuar commented Jun 6, 2023

Is there another way to fix this?

@JamesNK
Copy link
Member Author

JamesNK commented Jun 12, 2023

I don't know of one. I think a new feature is needed in HttpClient.

@Khazuar
Copy link

Khazuar commented Jun 13, 2023

As far as I can tell, there is no finalizer/destructor defined for HttpContent so far (though the infrastructure for a proper Dispose-Pattern is in place, even calling GC.SuppressFinalize). Couldn't we just add a destructor to the HttpContentWrapper? It would only fire if the user doesn't call Dispose and would guarantee that SubchannelCallTracker.Complete is called eventually. And for those who properly dispose of their response it will even work quite timely and efficiently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants