PicoContainer
  1. PicoContainer
  2. PICO-190

Reworked CyclicDependency detection

    Details

    • Type: Improvement Improvement
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1-1-beta-2
    • Component/s: None
    • Labels:
      None
    • Number of attachments :
      2

      Description

      hi,

      didn't want to commit this without having heard your opinions.

      The current InstantiatingComponentAdapters aren't thread-safe because the instantiating/verifying booleans will confuse pico and throw CyclicDependencyException whenever 2 threads try to instantiate a component at the same time. (Demonstrated in http://cvs.picocontainer.codehaus.org/java/picocontainer/src/test/org/picocontainer/defaults/SynchronizedComponentAdapterTestCase.java?rev=1.4&view=auto)

      However CyclicDependency usually only occur in testing/development since they either get eliminated by code changes or container magic. In live system its usually not needed to detect a cyclic dependency. Therefore I extracted this concern and moved it to a delegating ComponentAdapter which will take care of the detection.

      One side-effect is that the CyclicDependencyException now reports the dependencies in the order they would have been instantiated and not everything that is on the failing component list. IMHO its way clearer and easier to understand where the cycle occurs.

      To not break any expecting behavior before v1.0 we could have ConstructorInjectionComponentAdapterFactory wrap its Adapter in the CyclicDependencyDetectionAdapter by default.

      Let me know what you think. Assign this to me if you want it to be applied.

      (Patch + Adapter following).

      PS: it might be possible to make cyclic dependencies go away if the Adapter would automatically return a dynamic proxy instead of the real component and hotswap it after the instantiation is done. Only for interfaces tho.

      1. CyclicDependencyDetectionAdapter.java
        2 kB
        Thomas Heller
      2. patch.txt
        8 kB
        Thomas Heller

        Activity

        Thomas Heller made changes -
        Field Original Value New Value
        Attachment patch.txt [ 12154 ]
        Thomas Heller made changes -
        Thomas Heller made changes -
        Comment [ 20005 ]
        Thomas Heller made changes -
        Summary Reworked CyclicDepency detection Reworked CyclicDependency detection
        Aslak Hellesøy made changes -
        Comment [ 20030 ]
        Hide
        Paul Hammant added a comment -

        What would the consequences be of synchronizing on the component key - i.e. synchronize(compKey)

        { .. }

        instead of the soltion provided ?

        Show
        Paul Hammant added a comment - What would the consequences be of synchronizing on the component key - i.e. synchronize(compKey) { .. } instead of the soltion provided ?
        Hide
        Thomas Heller added a comment -

        There are several alternatives to this approach and all will work, however this implementation had 2 requirements it needed to clean for me:

        1) Report where the cycle occurs and which components where involved and NOT report the constructor that failed to instantiate because we had problem somewhere.

        eg. "C -> B -> A -> C failed" and not "C failed because B is a MissingDependency"

        2) Reusable for every type of component Instantiation, even for mixed Setter/Constructor/GetterInjection.

        Show
        Thomas Heller added a comment - There are several alternatives to this approach and all will work, however this implementation had 2 requirements it needed to clean for me: 1) Report where the cycle occurs and which components where involved and NOT report the constructor that failed to instantiate because we had problem somewhere. eg. "C -> B -> A -> C failed" and not "C failed because B is a MissingDependency" 2) Reusable for every type of component Instantiation, even for mixed Setter/Constructor/GetterInjection.
        Hide
        Konstantin Pribluda added a comment -

        This was problem in nanocontainer scripting for web wnvironment,
        and this is solved by adding synchronisation on container population.

        Though whole adapter/factory stuff needs discussion...

        Show
        Konstantin Pribluda added a comment - This was problem in nanocontainer scripting for web wnvironment, and this is solved by adding synchronisation on container population. Though whole adapter/factory stuff needs discussion...
        Hide
        Jörg Schaible added a comment -

        The cycle check is thread-safe now.

        Show
        Jörg Schaible added a comment - The cycle check is thread-safe now.
        Jörg Schaible made changes -
        Fix Version/s 1-1-beta-2 [ 11243 ]
        Status Open [ 1 ] Closed [ 6 ]
        Resolution Fixed [ 1 ]
        Aslak Hellesøy made changes -
        Comment [ 30526 ]

          People

          • Assignee:
            Unassigned
            Reporter:
            Thomas Heller
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: