Re: [PR] Fix race conditions in apache.beam.sdks.schemas [beam]

[via GitHub]

gemini-code-assist[bot] commented on code in PR #35752:
URL: https://github.com/apache/beam/pull/35752#discussion_r2246092819


##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/CachingFactory.java:
##########
@@ -45,10 +45,16 @@ public CachingFactory(@UnknownInitialization 
Factory<CreatedT> innerFactory) {
   }
 
   private ConcurrentHashMap<TypeDescriptor<?>, CreatedT> getCache() {
-    if (cache == null) {
-      cache = new ConcurrentHashMap<>();
+    ConcurrentHashMap<TypeDescriptor<?>, CreatedT> value = cache;
+    if (value == null) {
+      synchronized (this) {
+        value = cache;
+        if (value == null) {
+          cache = value = new ConcurrentHashMap<>();
+        }
+      }

Review Comment:
   ![medium](https://www.gstatic.com/codereviewagent/medium-priority.svg)
   
   While `synchronized(this)` is correct, consider synchronizing on a private, 
dedicated lock object (e.g., `private final Object lock = new Object();`). This 
encapsulates the lock, preventing external code from synchronizing on the same 
object, which could lead to unexpected contention or deadlocks.
   
   Since this class is not `final`, a subclass could introduce methods that 
synchronize on `this`, potentially causing interference. Using a private lock 
object would make the implementation more robust.



-- 
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: github-unsubscr...@beam.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org