Skip to content

Conversation

mayuki
Copy link
Contributor

@mayuki mayuki commented Mar 10, 2022

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Description

Fixes #40595

QueryParameterValueSupplier.ForType static method caches a QueryParameterValueSupplier in the Dictionary for each type.

The Dictionary for this cache is not locked and will be broken if multiple requests hit it on the first access.
This can also happen under heavy workloads or in cases such as health checks from load balancers at start-up.

It can be reproduced randomly by starting the Blazor Server with the following code running in a separate process.

var httpClient = new HttpClient();
var tasks = Enumerable.Range(0, 10).Select(async x =>
{
    while (true)
    {
        try
        {
            await httpClient.GetStringAsync("http://localhost:5167/");
        }
        catch {}
    }
})
.ToArray();
await Task.WhenAll(tasks);
System.IndexOutOfRangeException: Index was outside the bounds of the array.
   at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
   at Microsoft.AspNetCore.Components.Routing.QueryParameterValueSupplier.ForType(Type componentType) in C:\Users\UserName\source\repos\GitHub\mayuki-aspnetcore\src\Components\Components\src\Routing\QueryParameterValueSupplier.cs:line 35
   at Microsoft.AspNetCore.Components.RouteView.RenderPageWithParameters(RenderTreeBuilder builder) in C:\Users\UserName\source\repos\GitHub\mayuki-aspnetcore\src\Components\Components\src\RouteView.cs:line 95
   at Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder.AddContent(Int32 sequence, RenderFragment fragment) in C:\Users\UserName\source\repos\GitHub\mayuki-aspnetcore\src\Components\Components\src\Rendering\RenderTreeBuilder.cs:line 113
   at BlazorApp4.Shared.MainLayout.BuildRenderTree(RenderTreeBuilder __builder) in C:\Users\UserName\source\repos\Sandbox\BlazorApp4\BlazorApp4\Shared\MainLayout.razor:line 16
   at Microsoft.AspNetCore.Components.ComponentBase.<.ctor>b__6_0(RenderTreeBuilder builder) in C:\Users\UserName\source\repos\GitHub\mayuki-aspnetcore\src\Components\Components\src\ComponentBase.cs:line 40
   at Microsoft.AspNetCore.Components.Rendering.ComponentState.RenderIntoBatch(RenderBatchBuilder batchBuilder, RenderFragment renderFragment, Exception& renderFragmentException) in C:\Users\UserName\source\repos\GitHub\mayuki-aspnetcore\src\Components\Components\src\Rendering\ComponentState.cs:line 69

@mayuki mayuki requested a review from a team as a code owner March 10, 2022 11:52
@ghost ghost added area-blazor Includes: Blazor, Razor Components community-contribution Indicates that the PR has been added by a community member labels Mar 10, 2022
@mayuki mayuki changed the title Lock the cache dictionary for the type of QueryParameterValueSupplier Fix race condition when caching for a type of QueryParameterValueSupplier Mar 10, 2022
Co-authored-by: Pranav K <prkrishn@hotmail.com>
Copy link
Contributor

@mkArtakMSFT mkArtakMSFT 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 your contribution, @mayuki!

@javiercn feel free to merge if you have no further feedback.

@javiercn javiercn merged commit 38865d0 into dotnet:main Mar 11, 2022
@ghost ghost added this to the 7.0-preview3 milestone Mar 11, 2022
@javiercn
Copy link
Member

/backport to release/6.0

@github-actions
Copy link
Contributor

Started backporting to release/6.0: https://github.com/dotnet/aspnetcore/actions/runs/1968359849

@github-actions
Copy link
Contributor

@javiercn an error occurred while backporting to release/6.0, please check the run log for details!

Error: @javiercn is not a repo collaborator, backporting is not allowed.

@javiercn
Copy link
Member

@Pilchie not sure why my permissions don't work?

javiercn pushed a commit that referenced this pull request Mar 11, 2022
…ValueSupplier (#40636)

* Use a concurrent dictionary to avoid race conditions.
@mayuki mayuki deleted the fix-QueryParameterValueSupplier-racecondition branch March 11, 2022 11:21
@mkArtakMSFT
Copy link
Contributor

@wtgodbe do you know what permissions are needed for the backport bot to succeed?
I believe @javiercn is a collaborator, how otherwise he would merge this PR?

Error: @javiercn is not a repo collaborator, backporting is not allowed.

@wtgodbe
Copy link
Member

wtgodbe commented Mar 11, 2022

@javiercn You have to flip the visibility if your dotnet github org membership to public here: https://github.com/orgs/dotnet/people

@javiercn
Copy link
Member

@wtgodbe thanks

mkArtakMSFT pushed a commit that referenced this pull request Mar 11, 2022
…ValueSupplier (#40636) (#40663)

## Description

We were using a regular dictionary on an internal cache that can be accessed concurrently.

#40595

## Customer Impact

If two or more threads try to populate the cache concurrently, it can cause the app to fail.

## Regression?

- [ ] Yes
- [X] No

[If yes, specify the version the behavior has regressed from]

## Risk

- [ ] High
- [ ] Medium
- [X] Low

It's switching the underlying dictionary type used.

## Verification

- [ ] Manual (required)
- [X] Automated

We have extensive E2E and unit tests for this behavior.

## Packaging changes reviewed?

- [ ] Yes
- [ ] No
- [X] N/A

Co-authored-by: Mayuki Sawatari <mayuki+github@misuzilla.org>
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blazor Server App fails to restart when restarted by IIS
5 participants