Copilot commented on code in PR #15771:
URL: https://github.com/apache/grails-core/pull/15771#discussion_r3483843698
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormEnhancer.groovy:
##########
@@ -261,13 +309,31 @@ class GormEnhancer implements Closeable {
* @throws IllegalStateException if no instance API is found for the type
*/
static <D> GormInstanceApi<D> findInstanceApi(Class<D> entity, String
qualifier = findTenantId(entity)) {
- def instanceApi =
INSTANCE_APIS.get(qualifier)?.get(NameUtils.getClassName(entity))
+ String className = NameUtils.getClassName(entity)
+ def instanceApi = INSTANCE_APIS.get(qualifier)?.get(className)
+ if (instanceApi == null) {
+ instanceApi = initializeInstanceApi(entity, qualifier, className)
+ }
if (instanceApi == null) {
throw stateException(entity)
}
return instanceApi
}
+ private static <D> GormInstanceApi<D> initializeInstanceApi(Class<D>
entity, String qualifier, String className) {
+ GormEnhancer enhancer = findEnhancer(entity, qualifier, className)
+ if (enhancer == null) {
+ return null
+ }
+ GormInstanceApi<D> instanceApi =
INSTANCE_APIS.get(qualifier)?.get(className)
+ if (instanceApi != null) {
+ return instanceApi
+ }
+ instanceApi = enhancer.getInstanceApi(entity, qualifier)
+ INSTANCE_APIS.get(qualifier).put(className, instanceApi)
+ return instanceApi
Review Comment:
Lazy initialization is not atomic here either; concurrent calls can allocate
multiple `GormInstanceApi` instances for the same (qualifier, className).
Prefer an atomic `computeIfAbsent` to prevent redundant allocations under load.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormEnhancer.groovy:
##########
@@ -384,6 +473,7 @@ class GormEnhancer implements Closeable {
@CompileStatic
void close() throws IOException {
removeConstraints()
+ ENHANCERS.remove(datastore)
Review Comment:
`ENHANCERS.remove(datastore)` can remove a newer enhancer if more than one
`GormEnhancer` is created for the same `Datastore` instance (the map is keyed
only by datastore). Use the two-arg `remove(key, value)` so `close()` only
removes its own entry.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormEnhancer.groovy:
##########
@@ -278,11 +344,34 @@ class GormEnhancer implements Closeable {
* @throws IllegalStateException if no validation API is found for the type
*/
static <D> GormValidationApi<D> findValidationApi(Class<D> entity, String
qualifier = findTenantId(entity)) {
- def instanceApi =
VALIDATION_APIS.get(qualifier)?.get(NameUtils.getClassName(entity))
- if (instanceApi == null) {
+ String className = NameUtils.getClassName(entity)
+ def validationApi = VALIDATION_APIS.get(qualifier)?.get(className)
+ if (validationApi == null) {
+ validationApi = initializeValidationApi(entity, qualifier,
className)
+ }
+ if (validationApi == null) {
throw stateException(entity)
}
- return instanceApi
+ return validationApi
+ }
+
+ private static <D> GormValidationApi<D> initializeValidationApi(Class<D>
entity, String qualifier, String className) {
+ GormEnhancer enhancer = findEnhancer(entity, qualifier, className)
+ if (enhancer == null) {
+ return null
+ }
+ GormValidationApi<D> validationApi =
VALIDATION_APIS.get(qualifier)?.get(className)
+ if (validationApi != null) {
+ return validationApi
+ }
+ validationApi = enhancer.getValidationApi(entity, qualifier)
+ VALIDATION_APIS.get(qualifier).put(className, validationApi)
+ return validationApi
Review Comment:
Same race as the other lazy init helpers: concurrent calls can construct
multiple `GormValidationApi` instances for the same key. Use `computeIfAbsent`
to make initialization single-flight and avoid extra allocations.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormEnhancer.groovy:
##########
@@ -245,12 +276,29 @@ class GormEnhancer implements Closeable {
static <D> GormStaticApi<D> findStaticApi(Class<D> entity, String
qualifier = findTenantId(entity)) {
String className = NameUtils.getClassName(entity)
def staticApi = STATIC_APIS.get(qualifier)?.get(className)
+ if (staticApi == null) {
+ staticApi = initializeStaticApi(entity, qualifier, className)
+ }
if (staticApi == null) {
throw stateException(entity)
}
return staticApi
}
+ private static <D> GormStaticApi<D> initializeStaticApi(Class<D> entity,
String qualifier, String className) {
+ GormEnhancer enhancer = findEnhancer(entity, qualifier, className)
+ if (enhancer == null) {
+ return null
+ }
+ GormStaticApi<D> staticApi = STATIC_APIS.get(qualifier)?.get(className)
+ if (staticApi != null) {
+ return staticApi
+ }
+ staticApi = enhancer.getStaticApi(entity, qualifier)
+ STATIC_APIS.get(qualifier).put(className, staticApi)
+ return staticApi
Review Comment:
Lazy initialization is not atomic: two threads can both observe a missing
entry and create/put separate API instances, partially undermining the
allocation-reduction goal. Use an atomic `computeIfAbsent` (on the
per-qualifier map) to ensure only one API is constructed per (qualifier,
className).
--
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]
