You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tapestry.apache.org by th...@apache.org on 2022/08/23 12:08:38 UTC

[tapestry-5] branch master updated: When a non proxied object is beeing eager loaded registry has to instantiate all its constructor parameters. (#35)

This is an automated email from the ASF dual-hosted git repository.

thiagohp pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/tapestry-5.git


The following commit(s) were added to refs/heads/master by this push:
     new 49f9fdc5f When a non proxied object is beeing eager loaded registry has to instantiate all its constructor parameters. (#35)
49f9fdc5f is described below

commit 49f9fdc5f5e9ba820d86f92d32e2a45221753ebd
Author: zenios <di...@gmail.com>
AuthorDate: Tue Aug 23 15:08:34 2022 +0300

    When a non proxied object is beeing eager loaded registry has to instantiate all its constructor parameters. (#35)
    
    If one of these parameters is an eager loaded services (Not yet loaded) registry will create a proxy for it.
    Because the proxy is created, that eager load service will not be eager loaded since its already registered as a realized service.
    
    These issue is related with the following
    
    1. https://dev.tapestry.apache.narkive.com/m806StSk/strange-behavour-of-eagerload
    2. https://issues.apache.org/jira/browse/TAPESTRY-226.
---
 .../apache/tapestry5/ioc/internal/ModuleImpl.java  | 33 +++++++++++++---------
 .../src/test/groovy/ioc/specs/EagerLoadSpec.groovy |  8 ++++--
 ...agerLoadService.java => EagerLoadService1.java} |  2 +-
 ...ServiceImpl.java => EagerLoadService1Impl.java} |  6 ++--
 ...agerLoadService.java => EagerLoadService2.java} |  2 +-
 ...ServiceImpl.java => EagerLoadService2Impl.java} |  8 ++----
 .../tapestry5/ioc/test/EagerProxyReloadModule.java |  9 ++++--
 .../ioc/test/NonProxiedEagerLoadService.java       |  7 +++++
 8 files changed, 48 insertions(+), 27 deletions(-)

diff --git a/tapestry-ioc/src/main/java/org/apache/tapestry5/ioc/internal/ModuleImpl.java b/tapestry-ioc/src/main/java/org/apache/tapestry5/ioc/internal/ModuleImpl.java
index c2233c3f9..da90942f8 100644
--- a/tapestry-ioc/src/main/java/org/apache/tapestry5/ioc/internal/ModuleImpl.java
+++ b/tapestry-ioc/src/main/java/org/apache/tapestry5/ioc/internal/ModuleImpl.java
@@ -116,6 +116,12 @@ public class ModuleImpl implements Module
      */
     private final Map<String, Object> services = CollectionFactory.newCaseInsensitiveMap();
 
+
+    /**
+     * EagerLoadProxies collection into which proxies for eager loaded services are added. Guarded by BARRIER
+     */
+    private final Collection<EagerLoadServiceProxy> eagerLoadProxies = CollectionFactory.newList();
+
     private final Map<String, ServiceDef3> serviceDefs = CollectionFactory.newCaseInsensitiveMap();
 
     /**
@@ -161,7 +167,7 @@ public class ModuleImpl implements Module
         // RegistryImpl should already have checked that the service exists.
         assert def != null;
 
-        Object service = findOrCreate(def, null);
+        Object service = findOrCreate(def);
 
         try
         {
@@ -225,10 +231,9 @@ public class ModuleImpl implements Module
      * Locates the service proxy for a particular service (from the service definition).
      *
      * @param def              defines the service
-     * @param eagerLoadProxies collection into which proxies for eager loaded services are added (or null)
      * @return the service proxy
      */
-    private Object findOrCreate(final ServiceDef3 def, final Collection<EagerLoadServiceProxy> eagerLoadProxies)
+    private Object findOrCreate(final ServiceDef3 def)
     {
         final String key = def.getServiceId();
 
@@ -247,7 +252,7 @@ public class ModuleImpl implements Module
 
                 if (result == null)
                 {
-                    result = create(def, eagerLoadProxies);
+                    result = create(def);
 
                     services.put(key, result);
                 }
@@ -284,8 +289,16 @@ public class ModuleImpl implements Module
                 for (ServiceDef3 def : serviceDefs.values())
                 {
                     if (def.isEagerLoad())
-                        findOrCreate(def, proxies);
+                        findOrCreate(def);
                 }
+
+                BARRIER.withWrite(new Runnable() {
+                    @Override
+                    public void run() {
+                        proxies.addAll(eagerLoadProxies);
+                        eagerLoadProxies.clear();
+                    }
+                });
             }
         };
 
@@ -294,10 +307,8 @@ public class ModuleImpl implements Module
 
     /**
      * Creates the service and updates the cache of created services.
-     *
-     * @param eagerLoadProxies a list into which any eager loaded proxies should be added
      */
-    private Object create(final ServiceDef3 def, final Collection<EagerLoadServiceProxy> eagerLoadProxies)
+    private Object create(final ServiceDef3 def)
     {
         final String serviceId = def.getServiceId();
 
@@ -376,11 +387,7 @@ public class ModuleImpl implements Module
 
                     registry.addRegistryShutdownListener(delegate);
 
-                    // Occasionally eager load service A may invoke service B from its service builder method; if
-                    // service B is eager loaded, we'll hit this method but eagerLoadProxies will be null. That's OK
-                    // ... service B is being realized anyway.
-
-                    if (def.isEagerLoad() && eagerLoadProxies != null)
+                    if (def.isEagerLoad())
                         eagerLoadProxies.add(delegate);
 
                     tracker.setStatus(serviceId, Status.VIRTUAL);
diff --git a/tapestry-ioc/src/test/groovy/ioc/specs/EagerLoadSpec.groovy b/tapestry-ioc/src/test/groovy/ioc/specs/EagerLoadSpec.groovy
index de47abe2b..73e81b7d8 100644
--- a/tapestry-ioc/src/test/groovy/ioc/specs/EagerLoadSpec.groovy
+++ b/tapestry-ioc/src/test/groovy/ioc/specs/EagerLoadSpec.groovy
@@ -7,7 +7,9 @@ class EagerLoadSpec extends AbstractRegistrySpecification {
   def "proxied service does eager load"() {
     expect:
 
-    EagerProxyReloadModule.eagerLoadServiceDidLoad == false
+    EagerProxyReloadModule.eagerLoadService1DidLoad == false
+    EagerProxyReloadModule.nonProxyEagerLoadServiceDidLoad == false
+    EagerProxyReloadModule.eagerLoadService2DidLoad == false
 
     when:
 
@@ -17,6 +19,8 @@ class EagerLoadSpec extends AbstractRegistrySpecification {
 
     then:
 
-    EagerProxyReloadModule.eagerLoadServiceDidLoad == true
+    EagerProxyReloadModule.eagerLoadService1DidLoad == true
+    EagerProxyReloadModule.nonProxyEagerLoadServiceDidLoad == true
+    EagerProxyReloadModule.eagerLoadService2DidLoad == true
   }
 }
diff --git a/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerLoadService.java b/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerLoadService1.java
similarity index 94%
copy from tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerLoadService.java
copy to tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerLoadService1.java
index 808fb8c36..2cce5732c 100644
--- a/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerLoadService.java
+++ b/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerLoadService1.java
@@ -14,7 +14,7 @@
 
 package org.apache.tapestry5.ioc.test;
 
-public interface EagerLoadService
+public interface EagerLoadService1
 {
 
 }
diff --git a/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerLoadServiceImpl.java b/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerLoadService1Impl.java
similarity index 81%
copy from tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerLoadServiceImpl.java
copy to tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerLoadService1Impl.java
index d2274d0c5..8217db323 100644
--- a/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerLoadServiceImpl.java
+++ b/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerLoadService1Impl.java
@@ -17,10 +17,10 @@ package org.apache.tapestry5.ioc.test;
 import org.apache.tapestry5.ioc.annotations.EagerLoad;
 
 @EagerLoad
-public class EagerLoadServiceImpl implements EagerLoadService
+public class EagerLoadService1Impl implements EagerLoadService1
 {
-    public EagerLoadServiceImpl()
+    public EagerLoadService1Impl()
     {
-        EagerProxyReloadModule.eagerLoadServiceDidLoad = true;
+        EagerProxyReloadModule.eagerLoadService1DidLoad = true;
     }
 }
diff --git a/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerLoadService.java b/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerLoadService2.java
similarity index 94%
rename from tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerLoadService.java
rename to tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerLoadService2.java
index 808fb8c36..020982502 100644
--- a/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerLoadService.java
+++ b/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerLoadService2.java
@@ -14,7 +14,7 @@
 
 package org.apache.tapestry5.ioc.test;
 
-public interface EagerLoadService
+public interface EagerLoadService2
 {
 
 }
diff --git a/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerLoadServiceImpl.java b/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerLoadService2Impl.java
similarity index 74%
rename from tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerLoadServiceImpl.java
rename to tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerLoadService2Impl.java
index d2274d0c5..24f60a84a 100644
--- a/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerLoadServiceImpl.java
+++ b/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerLoadService2Impl.java
@@ -14,13 +14,11 @@
 
 package org.apache.tapestry5.ioc.test;
 
-import org.apache.tapestry5.ioc.annotations.EagerLoad;
 
-@EagerLoad
-public class EagerLoadServiceImpl implements EagerLoadService
+public class EagerLoadService2Impl implements EagerLoadService2
 {
-    public EagerLoadServiceImpl()
+    public EagerLoadService2Impl()
     {
-        EagerProxyReloadModule.eagerLoadServiceDidLoad = true;
+        EagerProxyReloadModule.eagerLoadService2DidLoad = true;
     }
 }
diff --git a/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerProxyReloadModule.java b/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerProxyReloadModule.java
index 1ba7d39f9..5375fa7c1 100644
--- a/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerProxyReloadModule.java
+++ b/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/EagerProxyReloadModule.java
@@ -16,12 +16,17 @@ package org.apache.tapestry5.ioc.test;
 
 import org.apache.tapestry5.ioc.ServiceBinder;
 
+//@ImportModule(EagerProxy2ReloadModule.class)
 public class EagerProxyReloadModule
 {
-    public static boolean eagerLoadServiceDidLoad;
+    public static boolean eagerLoadService1DidLoad;
+    public static boolean nonProxyEagerLoadServiceDidLoad;
+    public static boolean eagerLoadService2DidLoad;
 
     public static void bind(ServiceBinder binder)
     {
-        binder.bind(EagerLoadService.class, EagerLoadServiceImpl.class);
+        binder.bind(EagerLoadService1.class, EagerLoadService1Impl.class);
+        binder.bind(NonProxiedEagerLoadService.class).eagerLoad();
+        binder.bind(EagerLoadService2.class, EagerLoadService2Impl.class).eagerLoad();
     }
 }
diff --git a/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/NonProxiedEagerLoadService.java b/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/NonProxiedEagerLoadService.java
new file mode 100644
index 000000000..60d716da1
--- /dev/null
+++ b/tapestry-ioc/src/test/java/org/apache/tapestry5/ioc/test/NonProxiedEagerLoadService.java
@@ -0,0 +1,7 @@
+package org.apache.tapestry5.ioc.test;
+
+public class NonProxiedEagerLoadService {
+    public NonProxiedEagerLoadService(EagerLoadService2 service2) {
+        EagerProxyReloadModule.nonProxyEagerLoadServiceDidLoad = true;
+    }
+}