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

Mitigate race condition in EventSource_ConnectionPoolAtMaxConnection_LogsRequestLeftQueue test #55729

Merged
merged 1 commit into from
Jul 16, 2021

Conversation

alnikola
Copy link
Contributor

There is a unavoidable race condition between updating event counters and reading their values in WaitForEventCountersAsync. It waits for at least 2 sets of EventCounter event groups to ensure the last group is captured after any actual work (tests check that counters reset back to 0 if there is nothing happening).
Since it looks for requests-started which occurs before http11-requests-queue-duration event, what it may see is only the tail of the last group and the start of the second, without waiting for a fresh http11-requests-queue-duration. In this, the assert Assert.Equal(0, http11requestQueueDurations[^1]) on the line 549 will see a non-zero counter value and fail.

This PR changes the condition to < 3 to guarantee there is at least one full group of events in the window.

Co-authored by @MihaZupan

Fixes #46073

…_LogsRequestLeftQueue test

There is a unavoidable race condition between updating event counters and reading their values in WaitForEventCountersAsync. It waits for at least 2 sets of EventCounter event groups to ensure the last group is captured after any actual work (tests check that counters reset back to 0 if there is nothing happening).
Since it looks for `requests-started` which occurs before `http11-requests-queue-duration` event, what it may see is only the tail of the last group and the start of the second, without waiting for a fresh `http11-requests-queue-duration`. In this, the assert `Assert.Equal(0, http11requestQueueDurations[^1])` on the line 549 will see a non-zero counter value and fail.

This PR changes the condition to `< 3`  to guarantee there is at least one full group of events in the window.

Co-authored by @MihaZupan 

Fixes #46073
@alnikola alnikola requested a review from a team July 15, 2021 11:23
@ghost
Copy link

ghost commented Jul 15, 2021

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

Issue Details

There is a unavoidable race condition between updating event counters and reading their values in WaitForEventCountersAsync. It waits for at least 2 sets of EventCounter event groups to ensure the last group is captured after any actual work (tests check that counters reset back to 0 if there is nothing happening).
Since it looks for requests-started which occurs before http11-requests-queue-duration event, what it may see is only the tail of the last group and the start of the second, without waiting for a fresh http11-requests-queue-duration. In this, the assert Assert.Equal(0, http11requestQueueDurations[^1]) on the line 549 will see a non-zero counter value and fail.

This PR changes the condition to < 3 to guarantee there is at least one full group of events in the window.

Co-authored by @MihaZupan

Fixes #46073

Author: alnikola
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

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.

Thanks for putting up the PR.

cc: @geoffkizer this is the issue I mentioned that is possibly the reason you were seeing EventSource_ConnectionPoolAtMaxConnection_LogsRequestLeftQueue failures locally

@alnikola
Copy link
Contributor Author

/azp list

@alnikola
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@alnikola alnikola merged commit 3cdc4dd into main Jul 16, 2021
@alnikola alnikola deleted the alnikola/tel-logs-req-left-queue branch July 16, 2021 09:34
@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
3 participants