Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Http2: Fix remote server tests occassionally timing out in CI #39588

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2233,14 +2233,6 @@ public async Task PostAsync_Redirect_ResultingGetFormattedCorrectly(Configuratio
[Theory, MemberData(nameof(RemoteServersMemberData))]
public async Task PostAsync_RedirectWith307_LargePayload(Configuration.Http.RemoteServer remoteServer)
{
if (remoteServer.HttpVersion == new Version(2, 0))
{
// This is occasionally timing out in CI with SocketsHttpHandler and HTTP2, particularly on Linux
// Likely this is just a very slow test and not a product issue, so just increasing the timeout may be the right fix.
// Disable until we can investigate further.
return;
}

await PostAsync_Redirect_LargePayload_Helper(remoteServer, 307, true);
}

Expand Down Expand Up @@ -2276,7 +2268,10 @@ public async Task PostAsync_Redirect_LargePayload_Helper(Configuration.Http.Remo
// Compute MD5 of request body data. This will be verified by the server when it receives the request.
content.Headers.ContentMD5 = TestHelper.ComputeMD5Hash(contentBytes);

using (HttpClient client = CreateHttpClientForRemoteServer(remoteServer))
// Issue #39545: test occassionally timing on CI using default timeout of 100 seconds.
Copy link

@geoffkizer geoffkizer Jul 18, 2019

Choose a reason for hiding this comment

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

Unless we're leaving the issue open, we shouldn't tag the issue like this. Otherwise it looks like it's an ActiveIssue. It's fine to link to the issue for more info, but we should comment something like:

"Because [reasons], test can take very long to run, particularly in CI. See github issue [link] for more information."

That said, there isn't really any additional info in the github issue other than "this test takes a long time" so I'm not sure it's actually worth linking. The comment itself is pretty self explanatory.

Same comment below in the other test.

(edited for clarity)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, will adjust wording.

var timeout = TimeSpan.FromSeconds(150);

using (HttpClient client = CreateHttpClientForRemoteServer(remoteServer, timeout))
using (HttpResponseMessage response = await client.PostAsync(redirectUri, content))
{
try
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,12 +115,12 @@ protected static object GetUnderlyingSocketsHttpHandler(HttpClientHandler handle

public static readonly IEnumerable<object[]> RemoteServersMemberData = Configuration.Http.RemoteServersMemberData;

protected HttpClient CreateHttpClientForRemoteServer(Configuration.Http.RemoteServer remoteServer)
protected HttpClient CreateHttpClientForRemoteServer(Configuration.Http.RemoteServer remoteServer, TimeSpan? timeout = null)
{
return CreateHttpClientForRemoteServer(remoteServer, CreateHttpClientHandler());
return CreateHttpClientForRemoteServer(remoteServer, CreateHttpClientHandler(), timeout);
}

protected HttpClient CreateHttpClientForRemoteServer(Configuration.Http.RemoteServer remoteServer, HttpClientHandler httpClientHandler)
protected HttpClient CreateHttpClientForRemoteServer(Configuration.Http.RemoteServer remoteServer, HttpClientHandler httpClientHandler, TimeSpan? timeout = null)
{
HttpMessageHandler wrappedHandler = httpClientHandler;

Expand All @@ -133,6 +133,11 @@ protected HttpClient CreateHttpClientForRemoteServer(Configuration.Http.RemoteSe

var client = new HttpClient(wrappedHandler);
SetDefaultRequestVersion(client, remoteServer.HttpVersion);
if (timeout.HasValue)

Choose a reason for hiding this comment

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

Rather than pass this in and set it here, can we just have callers set it directly on the HttpClient? This is generally what we do for other settings on HttpClient.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, that should simplify the changes significantly.

{
client.Timeout = timeout.Value;
}

return client;
}

Expand Down
14 changes: 5 additions & 9 deletions src/System.Net.Http/tests/FunctionalTests/PostScenarioTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -167,14 +167,6 @@ public async Task PostLargeContentUsingContentLengthSemantics_Success(Configurat
[Theory, MemberData(nameof(RemoteServersMemberData))]
public async Task PostRewindableContentUsingAuth_NoPreAuthenticate_Success(Configuration.Http.RemoteServer remoteServer)
{
if (remoteServer.HttpVersion == new Version(2, 0))
{
// This is occasionally timing out in CI with SocketsHttpHandler and HTTP2, particularly on Linux
// Likely this is just a very slow test and not a product issue, so just increasing the timeout may be the right fix.
// Disable until we can investigate further.
return;
}

HttpContent content = new StreamContent(new CustomContent.CustomStream(Encoding.UTF8.GetBytes(ExpectedContent), true));
var credential = new NetworkCredential(UserName, Password);
await PostUsingAuthHelper(remoteServer, ExpectedContent, content, credential, false);
Expand Down Expand Up @@ -271,7 +263,11 @@ private async Task PostUsingAuthHelper(
HttpClientHandler handler = CreateHttpClientHandler();
handler.PreAuthenticate = preAuthenticate;
handler.Credentials = credential;
using (HttpClient client = CreateHttpClientForRemoteServer(remoteServer, handler))

// Issue #39545: test occassionally timing on CI using default timeout of 100 seconds.
var timeout = TimeSpan.FromSeconds(150);

using (HttpClient client = CreateHttpClientForRemoteServer(remoteServer, handler, timeout))
{
// Send HEAD request to help bypass the 401 auth challenge for the latter POST assuming
// that the authentication will be cached and re-used later when PreAuthenticate is true.
Expand Down