Skip to content

Conversation

MihaZupan
Copy link
Member

Potentially fixes #41078 and fixes #45204.
I ran these changes through CI with more iterations a few times in #89742 and didn't see any failures.
I think it's worth re-enabling these to see whether they are fixed or to get better exception info as to where they're failing.

The main differences are:

  • Using WhenAllCompletedOrAnyFailed instead of WhenAll in a bunch of places
  • Removed the manual readTimeout control of H2 connections - for whatever reason these tests were using timeout values up to 10x smaller than what we would otherwise default to for other tests.
  • Changed PrepareConnection test helper to no longer do a retry loop while waiting for the client setting ACK - just consider it implied based on the fact that reading & ACKing the settings is the first thing an HTTP2 connection does in SocketsHttpHandler.
  • Removed the expectedWarmUpTasks parameter that just seemed like a bug if we didn't immediately see the settings ACK - we would end up trying to receive more requests than we sent
  • Changed HandleAllPendingRequests and AcceptRequests to stop swallowing exceptions, so we can get more meaningful info if they do end up failing

@MihaZupan MihaZupan added this to the 8.0.0 milestone Aug 1, 2023
@MihaZupan MihaZupan requested a review from a team August 1, 2023 15:57
@MihaZupan MihaZupan self-assigned this Aug 1, 2023
@ghost
Copy link

ghost commented Aug 1, 2023

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

Issue Details

Potentially fixes #41078 and fixes #45204.
I ran these changes through CI with more iterations a few times in #89742 and didn't see any failures.
I think it's worth re-enabling these to see whether they are fixed or to get better exception info as to where they're failing.

The main differences are:

  • Using WhenAllCompletedOrAnyFailed instead of WhenAll in a bunch of places
  • Removed the manual readTimeout control of H2 connections - for whatever reason these tests were using timeout values up to 10x smaller than what we would otherwise default to for other tests.
  • Changed PrepareConnection test helper to no longer do a retry loop while waiting for the client setting ACK - just consider it implied based on the fact that reading & ACKing the settings is the first thing an HTTP2 connection does in SocketsHttpHandler.
  • Removed the expectedWarmUpTasks parameter that just seemed like a bug if we didn't immediately see the settings ACK - we would end up trying to receive more requests than we sent
  • Changed HandleAllPendingRequests and AcceptRequests to stop swallowing exceptions, so we can get more meaningful info if they do end up failing
Author: MihaZupan
Assignees: MihaZupan
Labels:

area-System.Net.Http

Milestone: 8.0.0

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@MihaZupan MihaZupan merged commit b5b3eec into dotnet:main Aug 1, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.