Skip to content

Associate tagged keys with entries so replacements are not evicted #61529

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Associate tagged keys with entries so replacements are not evicted #61529

wants to merge 9 commits into from
+59 −14

Conversation

profet23
Copy link

@profet23 profet23 commented Apr 17, 2025 •
edited
Loading

Associate tagged keys with entries so replacements are not evicted

  • 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.

MemoryOutputCacheStore will now only remove tags from the associated eviction, rather than all tags associated with the key.

Description

A previous PR (#43728) fixed a memory leak by removing tags upon eviction from the MemoryOutputCacheStore.

That fix created a bug where if an entry is replaced rather than evicted via timeout, the tags associated with the replacement would erroneously be deleted and the new entry would have no tags.

This PR associates each tag with a specific entry. When the PostEvictionCallback is run, only the associated tags with that specific eviction will be removed.

I based this on release/8.0 branch, I assume it can be merged into release/8.0, release/9.0, and main.

Fixes #61524

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Apr 17, 2025
@profet23 profet23 changed the base branch from main to release/8.0 April 17, 2025 03:33
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 17, 2025
@build-analysis build-analysis bot mentioned this pull request Apr 17, 2025
Open
@profet23 profet23 requested a review from gfoidl April 17, 2025 10:52
@martincostello
Copy link
Member

I based this on release/8.0 branch, I assume it can be merged into release/8.0, release/9.0, and main.

As a starting point this should target main. If it meets the servicing bar, then backporting it to other branches can be done by the team post-merge.

@profet23 profet23 force-pushed the profet23/issue_61524 branch from 3f4849d to ee019d9 Compare April 17, 2025 12:21
@profet23 profet23 requested a review from mkArtakMSFT as a code owner April 17, 2025 12:21
@profet23 profet23 changed the base branch from release/8.0 to main April 17, 2025 12:21
@profet23
Copy link
Author

I based this on release/8.0 branch, I assume it can be merged into release/8.0, release/9.0, and main.

As a starting point this should target main. If it meets the servicing bar, then backporting it to other branches can be done by the team post-merge.

Done.

@profet23 profet23 requested a review from gfoidl April 17, 2025 12:23
gfoidl
gfoidl approved these changes Apr 17, 2025
View reviewed changes
Copy link
Member

@gfoidl gfoidl left a comment

Choose a reason for hiding this comment

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

Code changes LGTM.

@build-analysis build-analysis bot mentioned this pull request Apr 17, 2025
Open
3 tasks
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Apr 25, 2025
@profet23
Copy link
Author

/azp run

@azure-pipelines Azure Pipelines
Copy link

Commenter does not have sufficient privileges for PR 61529 in repo dotnet/aspnetcore

@profet23
Copy link
Author

Anything more I need to do to get a review and keep this PR from going stale?

@ilonatommy
Copy link
Member

/azp run

@dotnet-policy-service dotnet-policy-service bot removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Apr 28, 2025
@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@profet23
Copy link
Author

profet23 commented May 1, 2025

@sebastienros any chance of a code review?

@profet23 profet23 requested a review from sebastienros May 1, 2025 16:42
@profet23
Copy link
Author

profet23 commented May 1, 2025

Test failures seem transient? And unrelated to this change. Any way to re-run?

@Rick-Anderson
Copy link
Contributor

Rick-Anderson commented May 1, 2025 •
edited
Loading

Test failures seem transient? And unrelated to this change. Any way to re-run?

I use the brute force approach, Close/open the PR.
I'm sure there's a cleaver way to run them again without doing that.

@profet23 profet23 closed this May 1, 2025
@profet23 profet23 reopened this May 1, 2025
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview5 milestone May 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebastienros sebastienros sebastienros approved these changes

@Rick-Anderson Rick-Anderson Rick-Anderson approved these changes

@gfoidl gfoidl gfoidl approved these changes

@tdykstra tdykstra Awaiting requested review from tdykstra

@wtgodbe wtgodbe Awaiting requested review from wtgodbe

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy BrennanConroy is a code owner

@halter73 halter73 Awaiting requested review from halter73

@JamesNK JamesNK Awaiting requested review from JamesNK

@mgravell mgravell Awaiting requested review from mgravell

@captainsafia captainsafia Awaiting requested review from captainsafia

@brunolins16 brunolins16 Awaiting requested review from brunolins16

@javiercn javiercn Awaiting requested review from javiercn

@mkArtakMSFT mkArtakMSFT Awaiting requested review from mkArtakMSFT

Assignees
No one assigned
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
10.0-preview5
Development

Successfully merging this pull request may close these issues.

MemoryOutputCacheStore doesn't respect "reason" for eviction and deletes tags erroneously
6 participants
@profet23 @martincostello @ilonatommy @Rick-Anderson @sebastienros @gfoidl