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

Fix queue count in rate limiters #90810

Merged
merged 1 commit into from
Aug 21, 2023
Merged

Conversation

BrennanConroy
Copy link
Member

Queue count can be updated by a cancellation request or inline when removing items from the queue (dispose a lease, replenish tokens, newest first kicking older queued items).

Since we just changed cancellation to not wait for callbacks to complete inline, that introduces a race where we don't update the queue count and loop until we removed enough items from the queue to add a new item.

do
{
    var item = queue.DequeueHead();
    item.Property; // <-- null ref
    // ...
}
while (_options.QueueLimit - _queueCount < permitCount);

Since the queue count might not be updated in the racy case, the DequeueHead() call will end up dequeuing an empty item (null) and null ref when access properties on it.

The end effect is that an incoming request will fail with a NRE instead of getting added to the queue successfully.

This PR changes the queue count to be updated by whoever has the lock when we want to update the queue count which avoids these issues.

@ghost
Copy link

ghost commented Aug 18, 2023

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

Issue Details

Queue count can be updated by a cancellation request or inline when removing items from the queue (dispose a lease, replenish tokens, newest first kicking older queued items).

Since we just changed cancellation to not wait for callbacks to complete inline, that introduces a race where we don't update the queue count and loop until we removed enough items from the queue to add a new item.

do
{
    var item = queue.DequeueHead();
    item.Property; // <-- null ref
    // ...
}
while (_options.QueueLimit - _queueCount < permitCount);

Since the queue count might not be updated in the racy case, the DequeueHead() call will end up dequeuing an empty item (null) and null ref when access properties on it.

The end effect is that an incoming request will fail with a NRE instead of getting added to the queue successfully.

This PR changes the queue count to be updated by whoever has the lock when we want to update the queue count which avoids these issues.

Author: BrennanConroy
Assignees: -
Labels:

area-System.Threading

Milestone: -

@BrennanConroy BrennanConroy merged commit 1b9e4e3 into dotnet:main Aug 21, 2023
105 checks passed
@BrennanConroy
Copy link
Member Author

/backport to release/8.0

@github-actions
Copy link
Contributor

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/5929263533

@BrennanConroy BrennanConroy deleted the brecon/nre branch August 21, 2023 17:04
radical pushed a commit that referenced this pull request Aug 21, 2023
jkoritzinsky added a commit that referenced this pull request Aug 22, 2023
jkoritzinsky added a commit that referenced this pull request Aug 22, 2023
radical added a commit that referenced this pull request Aug 23, 2023
* Bump main version to 9.0-alpha1

* Try bumping SdkBandVersion

* Bump the emscripten transport package

* Fix typo

* Update TFM to net9.0, minimum to net8.0, use RC1 nightly for SDK to avoid regressions in .NET 8.0 targets and clean up .NET 8->.NET 9 API compat baselining.

* Add compat suppressions for removed EOL TFM assets that will not be in the 9.0 variants of the packages.

* PR feedback

* Fix ILLink errors in the build.

* Update Versions.props

* PR feedback

* Don't use non-portable rids in src/tests infra

* Fix restore for Microsoft.NETCore.App.Runtime-based sfxprojs

* Fix libs build and expand where we set RunAnalyzers

* PR feedback

* Try fixing the package testing.

* Fix remaining restore issues.

* Patch preserialized resources to account for the major version bump (convert Version=8.0.0.0 to Version=9.0.0.0)

* Remove explicit hard-coding of net6.0 package testing

* Convert more references to net8.0 to net9.0

* Revert TFM changes

* Add workaround for RequiresPreviewFeatures and regenerate SLE compat suppressions.

* [wasm] Unpin sdk version, and change channel to 9.0 for WBT

* Revert "[wasm] Unpin sdk version, and change channel to 9.0 for WBT"

This reverts commit be2c615.

* Patch preserialized resources to account for the major version bump (convert Version=8.0.0.0 to Version=9.0.0.0)

* Suppress EventLog package validation failures to get allconfigurations passing.

* Fix queue count in rate limiters (#90810)

* Adding PACKAGE.md to System.Runtime.Caching package (#90701)

* Adding PACKAGE.md to System.Runtime.Caching package

* add Remarks

* move PACKAGE.md to src folder

* update per PR feedback

* add important block

* [wasm] Unpin sdk version, and change channel to 9.0 for WBT

* workloads-testing.targets: Ignore standard error/warning formats

Prompted by the following getting caught as a warning, and converted to
an error:

```
EXEC : error : dotnet-install: Failed to check the disk space. Installation will continue, but it may fail if you do not have [D:\a\_work\1\s\src\mono\wasm\Wasm.Build.Tests\Wasm.Build.Tests.csproj]
   enough disk space.
  dotnet-install: Extracting the archive.
```

* Force AssemblyVersion

* Revert changes.

* Remove the SLE suppressions again.

* Fix 9.0->8.0 replace

* Revert "Fix queue count in rate limiters (#90810)"

This reverts commit 1b9e4e3.

* Fix formatting

* Update Version.Details.xml for xlifftasks to try to fix prebuilt error.

---------

Co-authored-by: Larry Ewing <lewing@microsoft.com>
Co-authored-by: Jeremy Koritzinsky <jekoritz@microsoft.com>
Co-authored-by: Ankit Jain <radical@gmail.com>
Co-authored-by: Brennan <brecon@microsoft.com>
Co-authored-by: Hong Li <hongli@microsoft.com>
@ghost ghost locked as resolved and limited conversation to collaborators Sep 20, 2023
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.

2 participants