Axis2
  1. Axis2
  2. AXIS2-5267

Valid phase order configurations may result in invalid phase execution orders

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.5.4
    • Fix Version/s: None
    • Component/s: kernel
    • Labels:

      Description

      The partial ordering of Axis2 handlers is not fully enforced.

      Given a case with two handlers that ought to be run before a third, the system may fail as implemented.

      The current ordering system permits a configuration which is valid by inspection yet fails to enforce the described order. This property of the system emerges from both the configuration and the time at which phase ordering is checked.

      If my system relies on both A and B coming before C, I cannot state this dependency directly (see AXIS2-5266). However, I can invert this dependency and say A and B require C to follow.

      Because phase rules are only checked at insertion time, if A and B are inserted first, they are merely shifted to the beginning of the phase list (C is not yet present). The resultant phase list after the insertion of A and B might be: (B,A,X,Y,Z).

      If C specifies that B comes before it in its rule (which is a valid, albeit incomplete requirement), then C might be inserted immediately after B, resulting in this invalid phase ordering: (B,C,A,X,Y,Z).

      This example of valid configuration yielding invalid phase ordering constitutes a defect in the configuration.

      This defect could resolved by allowing the specification of multiple "before" and "after" handlers (see AXIS2-5266), creating a dependency graph, and resolving a valid ordering from the graph. A dependency graph may also generalize many of the validation steps currently performed into a cycle detection step. (Even phasefirst and phaselast rules amount to cycle detection if they are properly represented in the graph.) A dependency graph would also allow for confirmation that all dependencies are in place before a service is started.

      I believe this change would make the system more robust, and because the dependencies would be resolved in a linear chain at service startup, there's no ongoing performance penalty for creating/maintaining a more complex data structure.

        Activity

        James Grahn created issue -
        James Grahn made changes -
        Field Original Value New Value
        Labels handler module handler module phase
        Hide
        James Grahn added a comment -

        The attached patch addresses the issue by utilizing a modified DeployableChain class.

        The central components of the fix:
        1) The DeployableChain class is lightly modified to use a dirty bool, as suggested in the comments. Methods that alter the ordering are synchronized, though getChain() remains unsynchronized (though it will invoke the synchronized rebuild() when dirty). A remove method is added. A DeployableChainException is used to allow narrower catch clauses.
        2) The Phase class is modified to maintain a DeployableChain, rather than a List.
        3) A few more Phase methods must throw PhaseException, because of a possibility of encountering the dirty flag and failing to construct a valid chain.

        I'd call attention, too, to the handling of exceptions within MessageContext. This is, by far, my area of least confidence within this patch. It is a new circumstance: figuring out what to do when the handler ordering fails.


        TESTING NOTES:
        1) Presently, I have run and passed all unit tests in axis2-kernel. I have not yet run the tests for all components of axis2.
        2) No new unit tests have been added, and DeployableChain likely requires additional testing, with its new feature sets.

        I thought it best to submit as soon as I had basic confirmation that things worked. Will resubmit as more testing is done, others welcome to comment/test too.

        Show
        James Grahn added a comment - The attached patch addresses the issue by utilizing a modified DeployableChain class. The central components of the fix: 1) The DeployableChain class is lightly modified to use a dirty bool, as suggested in the comments. Methods that alter the ordering are synchronized, though getChain() remains unsynchronized (though it will invoke the synchronized rebuild() when dirty). A remove method is added. A DeployableChainException is used to allow narrower catch clauses. 2) The Phase class is modified to maintain a DeployableChain, rather than a List. 3) A few more Phase methods must throw PhaseException, because of a possibility of encountering the dirty flag and failing to construct a valid chain. I'd call attention, too, to the handling of exceptions within MessageContext. This is, by far, my area of least confidence within this patch. It is a new circumstance: figuring out what to do when the handler ordering fails. — TESTING NOTES: 1) Presently, I have run and passed all unit tests in axis2-kernel. I have not yet run the tests for all components of axis2. 2) No new unit tests have been added, and DeployableChain likely requires additional testing, with its new feature sets. I thought it best to submit as soon as I had basic confirmation that things worked. Will resubmit as more testing is done, others welcome to comment/test too.
        James Grahn made changes -
        James Grahn made changes -
        Comment [ I am developing a patch for this issue using a lightly modified version of the DeployableChain class.

        The central components of the fix:
        1) The DeployableChain class is lightly modified to use a dirty bool, as suggested in the comments. Methods that alter the ordering are synchronized, though getChain() remains unsynchronized (though it will invoke the synchronized rebuild() when dirty). A remove method is added. A DeployableChainException is used to allow narrower catch clauses.
        2) The Phase class is modified to maintain a DeployableChain, rather than a List.
        3) A few more Phase methods must throw PhaseException, because of a possibility of encountering the dirty flag and failing to construct a valid chain.

        I will build the project over the weekend and attempt to repair any breakages afterward. After I've done so, I will attach the patch to this issue. Additional unit tests will follow in a separate attachment afterward. ]

          People

          • Assignee:
            Unassigned
            Reporter:
            James Grahn
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:

              Development