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 cache config bug #247

Merged
merged 9 commits into from
Jun 11, 2024
Merged

Fix cache config bug #247

merged 9 commits into from
Jun 11, 2024

Conversation

sidrao2006
Copy link
Owner

@sidrao2006 sidrao2006 commented Jun 10, 2024

Breaking Changes and Migration Guide

  1. If you have been using the package without modifying cacheStalePeriod or maxCacheObjects, no changes are required.
    However, any previously cached font files will be ignored and should be deleted by running the migration tool.

  2. If you have modified cacheStalePeriod or maxCacheObjects, you'll have to pass the same values to any method that downloads, caches or loads fonts, otherwise the provided configuration will be ignored.
    Any previously cached font files will continue to remain in their respective cache folders and will be used by the package.
    Running the migration tool will have no effect on these font files.

  3. A migration tool has been provided -> DynamicCachedFonts.runMigrationTool()


Original behaviour: A default cache manager with cache key 'DynamicCachedFontsFontCacheKey' was used which stored font files in a folder with the same name.
If a value for cacheStalePeriod or maxCacheObjects is provided which differs from the default value then a new instance of CacheManager would be created with the generated cacheKey.

Problems:

  1. Once a garbage collection event is run, _cacheManagers is destroyed and hence the package forgets about the modified CacheManager and attempts to locate the file in the default cache folder (as mentioned above).

  2. CacheManager requires a singleton approach. Since each cacheKey requires a unique instance of CacheManager, calling any method provided by the package before calling cacheFont/cacheFontStream would result in cacheStalePeriod, maxCacheObjects to be ignored when downloading and adding the font to cache.

Solutions:

  1. There will be no default instance of CacheManager, instead an instance will be created using cacheKey after any garbage collection events (which includes app restarts).

  2. Any method that downloads, caches or loads font will now accept cacheStalePeriod, maxCacheObjects . This ensures that an instance of CacheManager will be created only once with the provided config

Changes:

  • Require cacheStalePeriod, maxCacheObjects to be passed to any
    method that calls getCacheManager. As a result, if the above mentioned
    properties are modified by the user of the package at any point, the
    modified values should be passed to any method provided by this package
  • Require cacheStalePeriod, maxCacheObjects to be passed to getCacheManager
  • Remove handleCacheManager

See #233

Original behaviour: A default cache manager with cache key 'DynamicCachedFontsFontCacheKey' was used which stored font files in a folder with the same name.
If a value for `cacheStalePeriod` or `maxCacheObjects` is provided which differs from the default value then a new instance of `CacheManager` would be created with the generated `cacheKey`.

Problem: Once a garbage collecttion event is run, `_cacheManagers`  is destroyed and hence the package forgets about the modified `CacheManager` and attempts to locate the file in the default cache folder (as mentioned above).

Solution: There will be no default instance of `CacheManager`, instead an instance will be created using `cacheKey` after any garbage collection events (which includes app restarts). The first time a font is stored will be done using the cache config provided and for subsequent retrievals, `CacheManger` with `cacheKey` will be used and so the package will look for the font file in the right place.

See #233

BREAKING CHANGES:

This commit does not include any changes to the public API. However, as mentioned above,
font files which have already been cached under 'DynamicCachedFontsFontCacheKey' will no
longer be used. A migration tool will be provided to delete old font files.
Any font files under 'DynamicCachedFontsFontCacheKey' will be deleted.
@sidrao2006 sidrao2006 had a problem deploying to Test Lab Integration Test June 10, 2024 18:34 — with GitHub Actions Failure
@sidrao2006 sidrao2006 temporarily deployed to Actions Integration Test June 10, 2024 18:34 — with GitHub Actions Inactive
@sidrao2006 sidrao2006 temporarily deployed to Actions Integration Test June 10, 2024 18:34 — with GitHub Actions Inactive
@sidrao2006 sidrao2006 had a problem deploying to Actions Integration Test June 10, 2024 18:34 — with GitHub Actions Failure
@sidrao2006 sidrao2006 temporarily deployed to Actions Integration Test June 10, 2024 18:34 — with GitHub Actions Inactive
@sidrao2006 sidrao2006 temporarily deployed to Actions Integration Test June 10, 2024 18:34 — with GitHub Actions Inactive
@github-actions github-actions bot added the example Issues/PRs related to changes in example label Jun 10, 2024
**For package users with custom values for `cacheStalePeriod`, `maxCacheObjects`,
these values must be passed to any method provided by this package. Otherwise the
provided config values will be ignored.**

**For package users who didn't modify these properties, no changes are required.
The default values will be used.**

Changes:

- Require `cacheStalePeriod`, `maxCacheObjects` to be passed to any
method that calls `getCacheManager`. As a result, if the above mentioned
properties are modified by the user of the package at any point, the
modified values should be passed to any method provided by this package
- Require `cacheStalePeriod`, `maxCacheObjects` to be passed to `getCacheManager`
- Remove `handleCacheManager`

Problem: `CacheManager` requires a singleton approach. Since each `cacheKey`
requires a unique instance of `CacheManager`, calling any method provided by
the package before calling `cacheFont`/`cacheFontStream` would result in
`cacheStalePeriod`, `maxCacheObjects` to be ignored when downloading and
adding the font to cache.

Solution: By accepting `cacheStalePeriod`, `maxCacheObjects` as args in all
methods, the instance of `CacheManager` created will use the provided config.
@sidrao2006 sidrao2006 temporarily deployed to Actions Integration Test June 11, 2024 09:28 — with GitHub Actions Inactive
@sidrao2006 sidrao2006 had a problem deploying to Test Lab Integration Test June 11, 2024 09:28 — with GitHub Actions Failure
@sidrao2006 sidrao2006 temporarily deployed to Actions Integration Test June 11, 2024 09:28 — with GitHub Actions Inactive
@sidrao2006 sidrao2006 had a problem deploying to Actions Integration Test June 11, 2024 09:28 — with GitHub Actions Failure
@sidrao2006 sidrao2006 had a problem deploying to Actions Integration Test June 11, 2024 09:28 — with GitHub Actions Failure
@sidrao2006 sidrao2006 temporarily deployed to Actions Integration Test June 11, 2024 09:28 — with GitHub Actions Inactive
@sidrao2006 sidrao2006 temporarily deployed to Test Lab Integration Test June 11, 2024 09:46 — with GitHub Actions Inactive
@sidrao2006 sidrao2006 temporarily deployed to Actions Integration Test June 11, 2024 09:46 — with GitHub Actions Inactive
@sidrao2006 sidrao2006 temporarily deployed to Actions Integration Test June 11, 2024 09:46 — with GitHub Actions Inactive
@sidrao2006 sidrao2006 temporarily deployed to Actions Integration Test June 11, 2024 09:46 — with GitHub Actions Inactive
@sidrao2006 sidrao2006 had a problem deploying to Actions Integration Test June 11, 2024 09:46 — with GitHub Actions Failure
@sidrao2006 sidrao2006 temporarily deployed to Actions Integration Test June 11, 2024 09:46 — with GitHub Actions Inactive
@sidrao2006 sidrao2006 merged commit 3aff4d1 into main Jun 11, 2024
13 of 14 checks passed
@sidrao2006 sidrao2006 deleted the fix/multiple-cache-managers-flaw branch June 11, 2024 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
example Issues/PRs related to changes in example
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant