sbglasius commented on code in PR #16149:
URL: https://github.com/apache/grails-core/pull/16149#discussion_r3869527534
##########
grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java:
##########
@@ -774,4 +800,65 @@ private static Object unwrapGString(Object value) {
}
return value;
}
+
+ /**
+ * The data binding beans of a single {@link ApplicationContext}, resolved
on first use and kept for as long as
+ * that context is the one being bound against.
+ */
+ private static final class ContextBoundBeans {
+
+ private final ApplicationContext applicationContext;
+ private final CachedBean<DataBindingSourceRegistry>
dataBindingSourceRegistry =
+ new CachedBean<>(DataBindingSourceRegistry.BEAN_NAME,
DataBindingSourceRegistry.class);
+ private final CachedBean<MimeTypeResolver> mimeTypeResolver =
+ new CachedBean<>(MimeTypeResolver.BEAN_NAME,
MimeTypeResolver.class);
+ private final CachedBean<DataBinder> dataBinder =
+ new CachedBean<>(DATA_BINDER_BEAN_NAME, DataBinder.class);
+
+ private ContextBoundBeans(ApplicationContext applicationContext) {
+ this.applicationContext = applicationContext;
+ }
+
+ private DataBindingSourceRegistry getDataBindingSourceRegistry() {
+ return dataBindingSourceRegistry.get(applicationContext);
+ }
+
+ private MimeTypeResolver getMimeTypeResolver() {
+ return mimeTypeResolver.get(applicationContext);
+ }
+
+ private DataBinder getDataBinder() {
+ return dataBinder.get(applicationContext);
+ }
+ }
+
+ /**
+ * A singleton bean looked up at most once per {@link ApplicationContext}.
Each bean is resolved lazily so that
+ * asking for one of them never triggers the creation of another.
+ * <p>
+ * Both fields are volatile and {@code resolved} is written last, so a
thread which sees {@code resolved} also
+ * sees the bean it was resolved to. Two threads racing simply look the
same singleton up twice.
+ *
+ * @param <T> the type of the bean
+ */
+ private static final class CachedBean<T> {
+
+ private final String beanName;
+ private final Class<T> beanType;
+ private volatile T bean;
+ private volatile boolean resolved;
+
+ private CachedBean(String beanName, Class<T> beanType) {
+ this.beanName = beanName;
+ this.beanType = beanType;
+ }
+
+ private T get(ApplicationContext applicationContext) {
+ if (!resolved) {
+ bean = applicationContext.containsBean(beanName) ?
applicationContext.getBean(beanName, beanType) : null;
+ resolved = true;
+ }
+ return bean;
+ }
Review Comment:
If a `bean` isn't resolved yet, it never will be since a miss sets `bean =
null` and `resolved = true` together, so `!resolved` is `false` on every later
call regardless of whether the bean shows up afterward.
```
if (bean == null) {
bean = applicationContext.containsBean(beanName) ?
applicationContext.getBean(beanName, beanType) : null;
}
return bean;
}
```
and drop the resolved field entirely (no replacement needed since nothing
else reads it). `bean` is already volatile, so keying the check on `bean ==
null` keeps the same safe-publication guarantee the class doc calls out, while
letting a bean registered after the first miss be picked up on a later call
My answer is AI assisted, aka it found the potential bug, I described it.
--
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]
