borinquenkid commented on PR #16535: URL: https://github.com/apache/grails-core/pull/16535#issuecomment-6018994161
Reviewed. The approach looks sound to me. **Checked** - `GormRegistry.removeDatastore(Datastore)` exists on the PR head and covers `allDatastores`, `datastoresByQualifier`, `entityDatastores` and the three API registries, so the new call is valid. - Root cause is credible: the heap dump (167 `SessionFactoryImpl` / 287 `StandardServiceRegistryImpl`) and the per-spec counters match the two production leaks (children kept in `GormRegistry`, schema tenant `SessionFactory`s outside `connectionSources`). Live heap after full GC going from 465m to 231m is a strong result. - The tenant connection source is tracked before `getChildDatastore(...)` runs, so it is still closed if child creation throws. **Minor, non-blocking** 1. Hibernate 7: `closeSchemaTenantConnectionSources()` runs after `super.destroy()` inside the `try`, so if `super.destroy()` throws the tenant sources are skipped. Hibernate 5 calls it first; moving it to the start in H7 would match and be safer. 2. `unregisterChildDatastores()` iterates `datastoresByConnectionSource.values()` without holding the map's lock (a `synchronizedMap` in H7, a plain `LinkedHashMap` in H5). Only matters if `addTenantForSchema` races `destroy()`. 3. `removeDatastore()` calls `removeConstraints()` on every call, which removes the global `unique` constraint, so it now runs once per child. Harmless if the enhancer close already does it, but the effect is global if another datastore is live in the same JVM. 4. The full Hibernate 5 suite wasn't run locally, so I'd wait for CI there. Happy to approve once CI is green. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
