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

#2246 Review Core cache and redesign Regex caching #2251

Merged
merged 7 commits into from
Jan 17, 2025

Conversation

raman-m
Copy link
Member

@raman-m raman-m commented Jan 16, 2025

Fixes #2246

Proposed Changes

  • Added new useful method to the IOcelotCache<T> interface: bool TryGetValue(string, string, out T).
    As a result, the Ocelot.Cache.CacheManager package must be released❗

  • Refactored the DefaultMemoryCache<T> class to use concurrent collections for regions, as the previous ones were not thread-safe.

  • Proposed implementing a real cache in the UpstreamTemplatePatternCreator class to initialize the Pattern property of the class with a cached Regex object when the UpstreamPathTemplate is being created.
    Note that the design with a static concurrent dictionary was acceptable due to the constant number of routes (and Regex objects) in the collection. However, for dynamic routing scenarios, it is better to switch to a real caching design for future use.

  • ❗Resolved memory leak in the DownstreamUrlCreatorMiddleware class by refactoring the RemoveQueryStringParametersThatHaveBeenUsedInTemplate method, where all Regex objects were saved into _regex, an internal static collection❗ This faulty design was unfortunately released in version 23.4.0 as PR Best practices for regular expressions versus Regex performance review #1348❗ We must learn from this incident because sometimes optimizations for CPU may adversely affect memory consumption 🙈
    Ultimately, the algorithm was refactored to remove Regex in favor of StringBuilder.

  • Refactored the IUrlPathToUrlTemplateMatcher interface, the UrlMatch class, and the associated logic.

  • Added load test to reproduce the problem with a memory leak. It helped to develop the fix. It should not run in the CI/CD environment because it consumes high CPU resources; thus, the test is skipped.

@raman-m raman-m added bug Identified as a potential bug hotfix Gitflow: Hotfix issue, PR related to hotfix branch Caching Ocelot feature: Caching Core Ocelot Core related or system upgrade (not a public feature) labels Jan 16, 2025
@raman-m raman-m self-assigned this Jan 16, 2025
@raman-m raman-m requested review from RaynaldM and ggnaegi January 16, 2025 18:04
@raman-m
Copy link
Member Author

raman-m commented Jan 16, 2025

@donnytian Welcome to review!

@raman-m raman-m linked an issue Jan 16, 2025 that may be closed by this pull request
@raman-m raman-m added this to the November'24 milestone Jan 16, 2025
@raman-m raman-m merged commit 2fe4734 into release/23.4 Jan 17, 2025
1 check passed
@raman-m raman-m deleted the 2246-reproduce branch January 17, 2025 09:51
}
}

public bool TryGetValue(string key, string region, out T value)
Copy link
Member

Choose a reason for hiding this comment

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

Just asking, why should we have region as parameter since we don't use it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Regions are used in the Administration API to delete cached objects within specific region:

DELETE {adminPath}/outputcache/{region}
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This clears a region of the cache. If you are using a backplane, it will clear all instances of the cache!
Giving your the ability to run a cluster of Ocelots and cache over all of them in memory and clear them all at the same time, so just use a distributed cache.
The region is whatever you set against the **Region** field in the `FileCacheOptions <https://github.com/search?q=repo%3AThreeMammals%2FOcelot%20FileCacheOptions&type=code>`_ section of the Ocelot configuration.

Copy link
Member Author

Choose a reason for hiding this comment

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

Regions have been defined in the Caching Configuration since the early releases:

"Region": "europe-central",
"Header": "OC-Caching-Control",
"EnableContentHashing": false // my route has GET verb only, assigning 'true' for requests with body: POST, PUT etc.
}
In this example **TtlSeconds** is set to 15 which means the cache will expire after 15 seconds.
The **Region** represents a region of caching.

Target: Emptying cache region by Administration API or even by custom C# code

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally, the region name should be the route name, but we propose to assign a custom name. Consequently, the DefaultMemoryCache service could be redesigned and enhanced.

@ggnaegi
Copy link
Member

ggnaegi commented Jan 17, 2025

@raman-m ok, fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Identified as a potential bug Caching Ocelot feature: Caching Core Ocelot Core related or system upgrade (not a public feature) hotfix Gitflow: Hotfix issue, PR related to hotfix branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory leak in Regex caching logic
2 participants