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

Feat/add redis example #1

Merged
merged 30 commits into from
Dec 6, 2024
Merged

Feat/add redis example #1

merged 30 commits into from
Dec 6, 2024

Conversation

rnelson
Copy link
Owner

@rnelson rnelson commented Dec 4, 2024

No description provided.

@rnelson
Copy link
Owner Author

rnelson commented Dec 4, 2024

A lot of stuff broke with this. Most notably, I disabled local cache for the distributed thing forgetting that it's not always the case. We should only disable local cache when there is no IDistributedCache available.

In trying to fix it, I started putting everything in the DI container instead of doing the static instances everywhere. That broke it much further, because I don't have a DI container in the tests.

Need to figure out how to give xUnit a DI container and register things with it.

* Switch to properly using DI 🤦‍♂️

* [WIP] Partially fix tests. No compilation errors, but most tests fail

* [WIP] Fix some DI

* Allow the caller to send in a cancellation token

* Fix removal of items in CacheHelperTests

* Remove an impossible test
DI now handles the cache, we can't set it anymore

* [WIP] ?

* Fix SlowDownMiddlewareExtensionsTests/SlowDownOptionsTests

* Make many of the middleware integration tests work again

* [WIP] Attempting to force a 404

* Fix more tests

* Remove `AspNetTestServerFixture`, not used

* Make /err work!
@rnelson rnelson closed this Dec 5, 2024
@rnelson
Copy link
Owner Author

rnelson commented Dec 5, 2024

Closing PR for the moment. Finishing up some code coverage stuff, then I need to make sure the standalone still works fine then test this branch with Aspire/Redis/Docker again.

@rnelson rnelson self-assigned this Dec 5, 2024
@rnelson
Copy link
Owner Author

rnelson commented Dec 6, 2024

A lot of the library was rewritten, properly setting it up using dependency injection (as is the norm for ASP.NET Core, and how it was already built to be added). This fixed a few issues that cropped up in unit tests previously and greatly simplified a lot of stuff.

A package is being generated, a readme is written, public bits are mostly/all documented, local memory cache works great, unit tests all pass, and the library has 100% unit test code coverage.

The only remaining item is fixing a bug with the Redis example. When an IDistributedCache is available and pointing at a Redis instance, Redis is properly updated by the middleware. Unfortunately, the IMemoryCache is used and never refreshed from the distributed cache. What this results in is a first API using the shared cache will count requests and write that to Redis, but when the second API goes to do something it will check its memory cache (creating the entry if needed), use a value of 0, then write that to Redis. Requests to the two APIs just go back and forth overwriting one another, with each reading its in-memory cache first.

The in-memory cache can be disabled, but doing so makes non-distributed uses completely break because they have nowhere to store the data.

  • There is no Redis in unit tests as the middleware never knows Redis is involved.

I've tried having CacheHelper (the one bit of code that directly works with the HybridCache) get an IDistributedCache and check if it's null, enabling/disabling local memory cache accordingly, but the unit tests break horribly because the container expects a non-null instance for that to then be found.

I've asked on Bluesky (no responses yet) and dug through a bunch of the HybridCache code, including DefaultHybridCache.cs. I may just be missing something obvious. Quitting for the night and coming back with fresh eyes tomorrow.

cc @rbenoit this is the one and only thing left at this point, if you're free to work on it tomorrow go forth

@rnelson rnelson reopened this Dec 6, 2024
@rnelson rnelson merged commit 3f32430 into main Dec 6, 2024
2 checks passed
@rnelson rnelson deleted the feat/add-redis-example branch December 6, 2024 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant