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

Remove or redesign query cache #408

Open
jakubfijalkowski opened this issue Aug 2, 2022 · 2 comments
Open

Remove or redesign query cache #408

jakubfijalkowski opened this issue Aug 2, 2022 · 2 comments
Labels
area-cqrs Area: CQRS mechanisms bug Something isn't working enhancement New feature or request
Milestone

Comments

@jakubfijalkowski
Copy link
Member

The query cache is a problematic beast - it neither works correctly by default (the query key serializes every property with ToString, which fails if you have composite type in query) nor it works the way you always want it to (it caches whole Tasks, which means - exceptions; it also races to first entry in the cache, so you can still have two queries that are run simultaneously).

The main problem however is that it does not work with our current Contracts Generator. The ICacheQueryKeyProvider interface is not exposed by the KnownTypes in contracts (nor it should be), and the query that wants custom cache key needs to implement it directly, hence the contracts won't compile since the type, although marked as internal, is not.

We have two options:

  1. Redesign the query cache to be more predictable (e.g. always require cache key provider to be specified, e.g. on attribute).
  2. Get rid of it completely (I'm a fan of this option).
@jakubfijalkowski jakubfijalkowski added bug Something isn't working enhancement New feature or request area-cqrs Area: CQRS mechanisms labels Aug 2, 2022
@konradbartecki-45e6
Copy link

konradbartecki-45e6 commented Jan 16, 2023

There is also a problem with cache when queries can return proper, fresh data, but repositories can return old data.
Investigation shows that this issue was related to most likely validators being registered in dependency injection container earlier than the command/operation handler.

When repositories are used, there seem to be a separate memory cache per instance of repository and repositories nor the memory cache is a singleton and maybe they should be registered as singletons.

Possible workaround is to ensure that a single instance of the repository is used.

@jakubfijalkowski
Copy link
Member Author

@konradbartecki-45e6 That's somewhat separate issue to cache.

Investigation shows that this issue was related to most likely validators being registered in dependency injection container earlier than the command/operation handler.

Validators are registered as singletons by design - the performance penalty of instantiating the validator was too much, hence we decided to instantiate it once and register as singleton. This makes them perform well but also makes them somewhat tricker to use, as you need to resolve dependencies at runtime and you can't take them in validator's constructor (otherwise you will get singletons, and that will be a problem).

We're thinking about migrating to transient/scoped validators, but this requires newer versions of FluentValidation that we currently can't upgrade to (because of inability to implement RuleForAsync the way it was implemented previously).

When repositories are used, there seem to be a separate memory cache per instance of repository and repositories nor the memory cache is a singleton and maybe they should be registered as singletons.

Repositories are thin wrappers on EFCore's DbContext and follow the same principles. So if entity is tracked, you will never materialize it again. And by default, all entities are tracked. They also should not be singletons, because you want the cache to be per request (so - scoped), otherwise you will get strange stale entries across different requests/transactions. Also - MARS will bite very, very quickly :)

To sum up, the solution to this problem (at least for now) is to use service locator when getting validator dependencies.

@jakubfijalkowski jakubfijalkowski modified the milestones: vnext, vNext Feb 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-cqrs Area: CQRS mechanisms bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants