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

reenable ConnectTimeout_PlaintextStreamFilterTimesOut_Throws test #55982

Merged
merged 3 commits into from
Jul 21, 2021

Conversation

geoffkizer
Copy link
Contributor

I believe the flakiness is caused because in very slow environments (like certain CI machines), the cancellation token may fire before the server accepts the connection, which will cause the server logic to hang.

Fix this by using ConnectCallback to avoid creating a real connection at all.

Fixes #55931

@ghost
Copy link

ghost commented Jul 20, 2021

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

Issue Details

I believe the flakiness is caused because in very slow environments (like certain CI machines), the cancellation token may fire before the server accepts the connection, which will cause the server logic to hang.

Fix this by using ConnectCallback to avoid creating a real connection at all.

Fixes #55931

Author: geoffkizer
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@geoffkizer
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -100,24 +97,15 @@ public async Task ConnectTimeout_PlaintextStreamFilterTimesOut_Throws(bool useSs
handler.ServerCertificateCustomValidationCallback = TestHelper.AllowAllCertificates;
var socketsHandler = GetUnderlyingSocketsHttpHandler(handler);
socketsHandler.ConnectTimeout = TimeSpan.FromSeconds(1);
socketsHandler.ConnectCallback = (context, token) => new ValueTask<Stream>(new MemoryStream());
Copy link
Member

Choose a reason for hiding this comment

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

Would this test the connect timeout? In this case, we are already providing "connected" stream, right? e.g. skipping the ConnectAsync or Connect calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, since we are providing a ConnectCallback, we skip doing the actual ConnectAsync.

@geoffkizer
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

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

Nit: The releaseServer TaskCompletionSource isn't needed after this change.
Since we're not connecting, I assume the LoopbackServer could be removed as well.

@geoffkizer
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@geoffkizer geoffkizer merged commit 016851a into dotnet:main Jul 21, 2021
@geoffkizer geoffkizer deleted the fixplaintexttest branch July 21, 2021 22:39
@karelz karelz added this to the 6.0.0 milestone Jul 22, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression test: ConnectTimeout_PlaintextStreamFilterTimesOut_Throws
4 participants