[
https://issues.apache.org/jira/browse/AXIS2-5862?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Robert Lazarski resolved AXIS2-5862.
------------------------------------
Resolution: Fixed
Nine years on, the ticket still has no independent reproducer, but
the stack trace at Phase.flowComplete:361 is reachable in principle
and the fix is small enough that the do-no-harm case is clear. Rather
than chase a ghost we're hardening the loop boundary.
▎ What changed. One guard at the top of the flowComplete() unwind loop
in Phase.java:
▎ // AXIS2-5862: guard against a stale currentPhaseIndex that exceeds
this
▎ // phase's handler count. The documented symptom is an empty phase
being
▎ // reached via flowComplete() with a non-zero inbound index, which
makes
▎ // handlers.get(currentHandlerIndex - 1) throw
ArrayIndexOutOfBoundsException
▎ // and silently abort flowComplete() for every earlier phase.
Capping at
▎ // handlers.size() turns that crash into a safe no-op / truncated
unwind,
▎ // and has no effect on the normal path where the index is already
valid.
▎ if (currentHandlerIndex > handlers.size()) {
▎ currentHandlerIndex = handlers.size();
▎ }
▎ Why not the originally proposed patch. The ticket description
suggests swapping every setCurrentPhaseIndex / getCurrentPhaseIndex
inside Phase.java for setCurrentHandlerIndex / getCurrentHandlerIndex.
That would be a regression. Those two MessageContext fields have
historically misleading names, and the field Javadoc (and the actual
usage) confirms the semantics is opposite to what the names suggest:
▎ - currentPhaseIndex actually tracks the handler index within the
current Phase. Phase.invoke and Phase.flowComplete read/write it via
get/setCurrentPhaseIndex.
▎ - currentHandlerIndex actually tracks the Phase index within the
execution chain (every element of MessageContext.executionChain is a
Phase; Phase implements Handler). AxisEngine.invoke walks the chain
via get/setCurrentHandlerIndex (AxisEngine.java:231-272).
▎ Phase.invoke and Phase.flowComplete are internally consistent with
that split. The variable on line 346 being called currentHandlerIndex
while it reads getCurrentPhaseIndex() is confusing to the reader but
not wrong semantically — the local is the handler-within-phase index
either way. Applying the originally proposed patch would make Phase
read/write the chain-level index where it must read/write the
handler-within-phase index, and the normal success path would break.
▎ Follow-up commit c016d8f3 adds Javadoc to the MessageContext
field/accessor declarations and to the confusing local in
Phase.flowComplete, so future readers don't trip on the same thing.
▎ Tests added in
modules/kernel/test/org/apache/axis2/engine/PhaseFlowCompleteTest.java
(5 new, all green; full kernel suite 393/393 green):
> Handler / Phase Indexes incorrect?
> ----------------------------------
>
> Key: AXIS2-5862
> URL: https://issues.apache.org/jira/browse/AXIS2-5862
> Project: Axis2
> Issue Type: Bug
> Components: kernel
> Affects Versions: 1.7.4
> Reporter: Jeff Thomas
> Priority: Major
> Fix For: 2.0.1
>
>
> I believe there is a problem or rather multiple problems in the Phase/Handler
> flow indexing..but I am not 100% sure.
> In 'org.apache.axis2.engine.Phase':
> 1. During the Phase invocation:
> While cycling through the handlers, I would expect
> 'msgctx.setHandlerIndex(i+1)' but instead the phase index is incremented.
> {code:java}
> int handlersSize = handlers.size();
>
> for (int i= currentIndex; i < handlersSize; i++) {
> Handler handler = (Handler) handlers.get(i);
> InvocationResponse pi = invokeHandler(handler, msgctx);
>
> if (!pi.equals(InvocationResponse.CONTINUE)) {
> return pi;
> }
>
> // Set phase index to the next handler
> msgctx.setCurrentPhaseIndex(i+1);
> }
> {code}
> 2. During the phase 'flowComplete':
> I would expect here 'msgContext.getCurrenHandlerIndex()' and
> 'msgContext.setCurrentHandlerIndex(0)' instead.
> {code:java}
> // This will be non-zero if we failed during execution of one of the
> // handlers in this phase
> int currentHandlerIndex = msgContext.getCurrentPhaseIndex();
> if (currentHandlerIndex == 0) {
> currentHandlerIndex = handlers.size();
> } else {
> /*We need to set it to 0 so that any previous phases will execute
> all
> * of their handlers.*/
> msgContext.setCurrentPhaseIndex(0);
> }
> for (; currentHandlerIndex > 0; currentHandlerIndex--) {
> Handler handler = (Handler) handlers.get(currentHandlerIndex - 1);
> if (isDebugEnabled) {
> log.debug(msgContext.getLogIDString() + " Invoking
> flowComplete() for Handler '" +
> handler.getName() + "' in Phase '" + phaseName + "'");
> }
> handler.flowComplete(msgContext);
> }
> {code}
> This is currrently causing errors in our installation because we are
> seeing a currentPhaseIndex == 1, which forces the "currentHandlerIndex == 1";
> however the phase we are in has no handlers. This causes an
> OutOfBoundsException when it attempts to get the handler with index[0] (1-1).
> {noformat}
> java.lang.ArrayIndexOutOfBoundsException: 0
> at
> java.util.concurrent.CopyOnWriteArrayList.get(CopyOnWriteArrayList.java:368)
> at
> java.util.concurrent.CopyOnWriteArrayList.get(CopyOnWriteArrayList.java:377)
> at org.apache.axis2.engine.Phase.flowComplete(Phase.java:361)
> {noformat}
> Unfortunately I am not sure if there even more locations where HandlerIndex
> instead of PhaseIndex probably should have been used.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
