-
-
Notifications
You must be signed in to change notification settings - Fork 270
Description
We noticed, that our bean chache sometimes contains data of an other tenant.
We use a ThreadLocal for our tenant switching mechanism.
Normally (99.9%) we always make a tenant change in closed blocks (e.g. try-with-resources) and we normally do not pass out any entity of such a block.
small example for better understanding:
ThreadLocal<Integer> currentTenan =...;
currentTenant.set(1); // = ROOT;
try (var ctx = new Context(1000)) { // stores currentTenant value
// currentTenant == 1000
} // close will restore old value
// currentTenant == 1 (root)
try (var ctx = new Context(2000)) {
// currentTenant == 2000
try (var ctx2 = new Context(3000)) { // we support also nesting
// currentTenant == 3000
}
// currentTenant == 2000
}Unfortunately, there may be 0.1% where we have made a mistake in the code, where an entity is hand out in an other thread. (we did not find that spot, yet) This would be no problem, as long as no lazy load will occur. But there is #914 which guarantees, that the data is queried from the correct database. Unfortunately, the beanCache (tenantAwareKey e.g.) does not honor that value, that was provided for lazy-loading. So when reading from or writing to the beanCache we might hit the wrong cache and either read the wrong data or write the wrong data to the cache
We currently have the following theory:
- Somewhere in our code-base we partially load a bean
- This bean is passed to an other tenant/thread where a lazy load occurs (We did not found the causing code, yet, so we added a warning here: https://github.com/FOCONIS/ebean/blob/ebean-parent-14.8.1-FOC21/ebean-core/src/main/java/io/ebeaninternal/server/core/DefaultServer.java#L993)
- Ebean loads the correct data from the database (because of Fix multi-tenancy support such that it automatically propagates the tenantId for lazy loading #914) but puts it into the wrong cache (in our case, the root tenant)
- When the root tenant will access the same bean (with same ID) - it will get a cache hit with the wrong data.
I think tenant support was added later, so the ThreadLocal approach is no longer completely clean. Especially with the requirement from #914.
In my opinion, we could either prohibit lazy loading from working when the tenant changes, or - other instances are not allowed to rely on the value in CurrentTenantProvider and it must always be passed along during cache accesses.
BTW: If you're making any changes here, I've made the following improvements in the past:
- FEATURE: Tenant partitioned caches could improve cache-hit ratio #2956 - This does not rely on TenantAwareKey, but it has one cache for each tenant. It would be great, if this becomes the default.
- Your query-plan-capture does not support tenants, yet. I've a fix here: IMPROVED: Tenant support for query plans #2955 - but relies also on CurrentTeantProvider