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

Skip, quarantine and un-Quarantine #34564

Merged
merged 3 commits into from
Jul 21, 2021
Merged

Skip, quarantine and un-Quarantine #34564

merged 3 commits into from
Jul 21, 2021

Conversation

dougbu
Copy link
Member

@dougbu dougbu commented Jul 20, 2021

@dougbu dougbu requested review from Tratcher, TanayParikh and a team July 20, 2021 20:28
- un-quarantine `ContentLength_Received_MultipleDataFramesOverSize_Reset`, #33373 fix seems to have worked
- quarantine `LoggingConnectionMiddlewareCanBeAddedBeforeAndAfterHttps()`, #34561
- skip `LongPollingTransportOnReceiveGetsCalled()` because we can't quarantine Java tests, #34563
@dougbu dougbu force-pushed the dougbu/quarantinize branch from 7b9a3a3 to c6f2b06 Compare July 20, 2021 20:37
@dougbu dougbu enabled auto-merge (squash) July 20, 2021 20:47
dougbu added 2 commits July 20, 2021 15:16
- `@Ignore` is no longer supported w/ jUnit 5 / Juniper
@@ -14,6 +14,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.InMemory.FunctionalTests
public class LoggingConnectionMiddlewareTests : LoggedTest
{
[Fact]
[QuarantinedTest("https://github.com/dotnet/aspnetcore/issues/34561")]
Copy link
Member

@halter73 halter73 Jul 21, 2021

Choose a reason for hiding this comment

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

| [6.375s] Microsoft.AspNetCore.Server.Kestrel.Connections Debug: Connection id "0HMAB2I8425HF" accepted.
| [10.059s] Microsoft.AspNetCore.Server.Kestrel.Connections Debug: Connection id "0HMAB2I8425HF" started.

[20.479s] Microsoft.AspNetCore.Server.Kestrel.Https.Internal.HttpsConnectionMiddleware Debug: Authentication of the HTTPS connection timed out.

It looks like there might have been some serious threadpool starvation for the "accepted" and "started" logs to be ~4 seconds apart. The only thing between those two logs is a threadpool dispatch. I think that's what caused the handshake timeout and failed test.

"Accepted" log:

Log.ConnectionAccepted(connection.ConnectionId);
KestrelEventSource.Log.ConnectionQueuedStart(connection);
ThreadPool.UnsafeQueueUserWorkItem(kestrelConnection, preferLocal: false);

Which immediately dispatches to the "started" log:

void IThreadPoolWorkItem.Execute()
{
_ = ExecuteAsync();
}
internal async Task ExecuteAsync()
{
var connectionContext = _transportConnection;
try
{
KestrelEventSource.Log.ConnectionQueuedStop(connectionContext);
Logger.ConnectionStart(connectionContext.ConnectionId);
KestrelEventSource.Log.ConnectionStart(connectionContext);

I'm not sure we should be quarantining tests because of threadpool starvation caused by who-knows-what. This seems to just be the unlucky victim.

@dougbu
Copy link
Member Author

dougbu commented Jul 21, 2021

I'm not sure we should be quarantining tests because of threadpool starvation caused by who-knows-what. This seems to just be the unlucky victim.

Other tests in this class succeeded and the ThreadPool must not have been completely maxed out for that to happen. Is the timeout specified somewhere in the test❔ More generally, are you adding info for #34561 or suggesting a change to this PR❔

@dougbu dougbu merged commit ecf41a2 into main Jul 21, 2021
@dougbu dougbu deleted the dougbu/quarantinize branch July 21, 2021 01:12
@ghost ghost added this to the 6.0-rc1 milestone Jul 21, 2021
@dougbu
Copy link
Member Author

dougbu commented Jul 21, 2021

Never mind @halter73. Do as you wish w/ #34561; I suggest upping timeouts used in the test(s) if possible.

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.

3 participants