sailfin
  1. sailfin
  2. SAILFIN-1859

routes pushed by the AR are not used when canceling a proxybranch

    Details

    • Type: Bug Bug
    • Status: Reopened
    • Priority: Critical Critical
    • Resolution: Unresolved
    • Affects Version/s: 1.0
    • Fix Version/s: b31
    • Component/s: sip_container
    • Labels:
      None
    • Environment:

      Operating System: All
      Platform: All

    • Issuezilla Id:
      1,859

      Description

      routes pushed by the AR are not used when canceling a proxybranch

      The application router is allowed to push routes using the route and routeback
      directives.
      The problem is that when the CANCEL is created for an outgoing proxy, the routes
      pushed by the AR are NOT copied and the CANCEL fails miserably since it is
      routed to nodes that do not know the session.

      I've been looking into the proxy implementation and I think I see the
      problem with this.
      There are three copies of the request;

      • the original request before proxying
      • the cloned request that is passed to the proxybranch.
        This is the request which the application modifies (e.g., adds headers
        etc.). This is because when the application later receives responses, the
        getRequest() on these responses should return the original request and not the
        request as it might have been modified downstream.
      • the forward request, which is a cloned version of the proxybranch's
        request.
        This is the request that is actually dispatched. The container sets some
        stuff on this.

      The problem is that the ApplicationRouter modifies the forward request
      before it leaves the container.
      However, the cancel is based on the proxybranch request.

      Result; the cancel does not contain the AR's pushed routes.

      There might be related issues which have not been explored fully.

      E.g,, when the AR pushes an internal route, it is not actually added to the
      request, but instead made available to the application with the getPoppedRoute()
      call. For the CANCEL handling, when an application in the chain does a
      getPoppedRoute() it should see the same poppedRoute as when handling the invite
      request, but this is probably also broken due to this issue.

      Also for UACs (or B2Bs) it has not been investigated whether routes pushed by
      the AR are preserved for the cancel,

      The suggestion is to write a number of testcases with a route pushing AR and
      validate the various scenarios.

      The same issue exists in boh 1.0 and 2.0

        Activity

        Hide
        erikvandervelden added a comment -

        I'm trying a simple fix now.
        The fix is this, in addition to storing the cloned request, the proxybranch also
        stores the forward request.
        The cancel is build based on the forward request.

        This is a quick fix and not optimal from performance point of view.
        Comments from Robert:
        I don't see any obvious problems with your proposal of also keeping a
        reference to the forwarded request except more memory consumption. Maybe
        one could add a reference to a list of the pushed route headers instead
        of the total forwareded request. Remember to make sure that it should
        only be a reference to next internal proxy hop i.e if you have a chain
        of applications and AR is called many times and everytime it adds routes
        this adding of routes should be done in a general way and added for
        every step.

        — ProxyBranchImpl_SF1 2009-07-17 12:21:15.000000000 +0000
        +++ ProxyBranchImpl.java 2009-07-17 11:43:38.000000000 +0000
        @@ -129,6 +129,7 @@
        */
        private ClientTransaction clientTransaction;
        private ProxyBranchInternalRequest internalReq;
        + private transient SipServletRequestImpl _forward;

        /**

        • The constructor of the Proxy Branch implementation.
          @@ -641,51 +642,51 @@
          prepareProxyTo();

        // the request need to be cloned

        • final SipServletRequestImpl forward = (SipServletRequestImpl)
          getRequestImpl()
          + _forward = (SipServletRequestImpl) getRequestImpl()
          .clone();

        if (getRecordRoute())

        { // state Record Route in request to inform // that a RecordRoute header must be added - forward.indicateRecordRoute(); + _forward.indicateRecordRoute(); }

        if (getAddToPath())

        { // state Path in request to inform // that a Path header shall be added - forward.indicatePath(); + _forward.indicatePath(); }

        if (getProxyImpl().getNoCancel())

        { - forward.setNoCancel(); + _forward.setNoCancel(); }

        // JSR289: when a request is proxied, its routing directive should
        // be implicitly set to CONTINUE. See also issue 773.

        • forward.setImplicitRoutingDirective(SipApplicationRoutingDirective.CONTINUE,
          +
          _forward.setImplicitRoutingDirective(SipApplicationRoutingDirective.CONTINUE,
          null);
          // no need to copy region, expected to be set already

        • forward.setTransactionRequest(getRequestImpl());
          + _forward.setTransactionRequest(getRequestImpl());

        SipContainerThreadPool.getInstance().execute(new Callable() {
        public Object call() throws Exception {
        if (_log.isLoggable(Level.FINE))

        { _log.log(Level.FINE, - "Proxy request " + forward.toDebugString() + ", " + + "Proxy request " + _forward.toDebugString() + ", " + toString()); }

        // Activate the dialog lifecycle u-o-w

        • forward.getDialog().getDialogLifeCycle()
          + _forward.getDialog().getDialogLifeCycle()
          .setThreadLocalUnitOfWork();

        try

        { // push the request to the dispatcher - forward.setClientTransactionRegistrationListener(ProxyBranchImpl.this); - forward.popDispatcher().dispatch(forward); + _forward.setClientTransactionRegistrationListener(ProxyBranchImpl.this); + _forward.popDispatcher().dispatch(_forward); return null; }

        finally

        { @@ -794,7 +795,13 @@ }

        private SipServletRequestImpl createCancel() {

        • SipServletRequestImpl cancel = _request.createCancelImpl();
          + // quick hack to use forward to create a cancel, instead of the original
          request
          + // the reason is that the AR might have added routes to the forward,
          which are not
          + // added to the original request.
          + SipServletRequestImpl cancel = _forward.createCancelImpl();
          + // alt 2
          + // only copy the route headers from the _forward request to the cancel.
          +

        SipServletResponseImpl provisonal = getProvisionalResponse();

        Show
        erikvandervelden added a comment - I'm trying a simple fix now. The fix is this, in addition to storing the cloned request, the proxybranch also stores the forward request. The cancel is build based on the forward request. This is a quick fix and not optimal from performance point of view. Comments from Robert: I don't see any obvious problems with your proposal of also keeping a reference to the forwarded request except more memory consumption. Maybe one could add a reference to a list of the pushed route headers instead of the total forwareded request. Remember to make sure that it should only be a reference to next internal proxy hop i.e if you have a chain of applications and AR is called many times and everytime it adds routes this adding of routes should be done in a general way and added for every step. — ProxyBranchImpl_SF1 2009-07-17 12:21:15.000000000 +0000 +++ ProxyBranchImpl.java 2009-07-17 11:43:38.000000000 +0000 @@ -129,6 +129,7 @@ */ private ClientTransaction clientTransaction; private ProxyBranchInternalRequest internalReq; + private transient SipServletRequestImpl _forward; /** The constructor of the Proxy Branch implementation. @@ -641,51 +642,51 @@ prepareProxyTo(); // the request need to be cloned final SipServletRequestImpl forward = (SipServletRequestImpl) getRequestImpl() + _forward = (SipServletRequestImpl) getRequestImpl() .clone(); if (getRecordRoute()) { // state Record Route in request to inform // that a RecordRoute header must be added - forward.indicateRecordRoute(); + _forward.indicateRecordRoute(); } if (getAddToPath()) { // state Path in request to inform // that a Path header shall be added - forward.indicatePath(); + _forward.indicatePath(); } if (getProxyImpl().getNoCancel()) { - forward.setNoCancel(); + _forward.setNoCancel(); } // JSR289: when a request is proxied, its routing directive should // be implicitly set to CONTINUE. See also issue 773. forward.setImplicitRoutingDirective(SipApplicationRoutingDirective.CONTINUE, + _forward.setImplicitRoutingDirective(SipApplicationRoutingDirective.CONTINUE, null); // no need to copy region, expected to be set already forward.setTransactionRequest(getRequestImpl()); + _forward.setTransactionRequest(getRequestImpl()); SipContainerThreadPool.getInstance().execute(new Callable() { public Object call() throws Exception { if (_log.isLoggable(Level.FINE)) { _log.log(Level.FINE, - "Proxy request " + forward.toDebugString() + ", " + + "Proxy request " + _forward.toDebugString() + ", " + toString()); } // Activate the dialog lifecycle u-o-w forward.getDialog().getDialogLifeCycle() + _forward.getDialog().getDialogLifeCycle() .setThreadLocalUnitOfWork(); try { // push the request to the dispatcher - forward.setClientTransactionRegistrationListener(ProxyBranchImpl.this); - forward.popDispatcher().dispatch(forward); + _forward.setClientTransactionRegistrationListener(ProxyBranchImpl.this); + _forward.popDispatcher().dispatch(_forward); return null; } finally { @@ -794,7 +795,13 @@ } private SipServletRequestImpl createCancel() { SipServletRequestImpl cancel = _request.createCancelImpl(); + // quick hack to use forward to create a cancel, instead of the original request + // the reason is that the AR might have added routes to the forward, which are not + // added to the original request. + SipServletRequestImpl cancel = _forward.createCancelImpl(); + // alt 2 + // only copy the route headers from the _forward request to the cancel. + SipServletResponseImpl provisonal = getProvisionalResponse();
        Hide
        sankara added a comment -

        Assigning it to Binod.

        Show
        sankara added a comment - Assigning it to Binod.
        Hide
        binod added a comment -

        Fix description: Instead of saving the request object, the routes pushed were saved in a list and is used
        while creating the cancel. Checking in the fix.

        Checking in src/java/com/ericsson/ssa/sip/ProxyBranchImpl.java;
        /cvs/sailfin/sip-stack/src/java/com/ericsson/ssa/sip/ProxyBranchImpl.java,v <--
        ProxyBranchImpl.java
        new revision: 1.38; previous revision: 1.37
        done
        Checking in src/java/com/ericsson/ssa/sip/SipServletRequestImpl.java;
        /cvs/sailfin/sip-stack/src/java/com/ericsson/ssa/sip/SipServletRequestImpl.java,v <--
        SipServletRequestImpl.java
        new revision: 1.83; previous revision: 1.82
        done

        Show
        binod added a comment - Fix description: Instead of saving the request object, the routes pushed were saved in a list and is used while creating the cancel. Checking in the fix. Checking in src/java/com/ericsson/ssa/sip/ProxyBranchImpl.java; /cvs/sailfin/sip-stack/src/java/com/ericsson/ssa/sip/ProxyBranchImpl.java,v <-- ProxyBranchImpl.java new revision: 1.38; previous revision: 1.37 done Checking in src/java/com/ericsson/ssa/sip/SipServletRequestImpl.java; /cvs/sailfin/sip-stack/src/java/com/ericsson/ssa/sip/SipServletRequestImpl.java,v <-- SipServletRequestImpl.java new revision: 1.83; previous revision: 1.82 done
        Hide
        binod added a comment -

        Corrected the build number

        Show
        binod added a comment - Corrected the build number
        Hide
        binod added a comment -

        Modified the fix to avoid NPE.

        Checking in ProxyBranchImpl.java;
        /cvs/sailfin/sip-stack/src/java/com/ericsson/ssa/sip/ProxyBranchImpl.java,v <--
        ProxyBranchImpl.java
        new revision: 1.40; previous revision: 1.39
        done
        Checking in SipServletRequestImpl.java;
        /cvs/sailfin/sip-stack/src/java/com/ericsson/ssa/sip/SipServletRequestImpl.java,v <--
        SipServletRequestImpl.java
        new revision: 1.85; previous revision: 1.84
        done

        Show
        binod added a comment - Modified the fix to avoid NPE. Checking in ProxyBranchImpl.java; /cvs/sailfin/sip-stack/src/java/com/ericsson/ssa/sip/ProxyBranchImpl.java,v <-- ProxyBranchImpl.java new revision: 1.40; previous revision: 1.39 done Checking in SipServletRequestImpl.java; /cvs/sailfin/sip-stack/src/java/com/ericsson/ssa/sip/SipServletRequestImpl.java,v <-- SipServletRequestImpl.java new revision: 1.85; previous revision: 1.84 done
        Hide
        erikvandervelden added a comment -

        THe solution implemented in the end (see above) does not solve the original
        problem completely.
        The solution exists of storing all the pushed routes in a list.
        Then using the list to create route header in the cancel.

        if (routesForCancel != null && !routesForCancel.isEmpty()) {
        MultiLineHeader routeHdr = new MultiLineHeader(Header.ROUTE, true);
        for (Address route : routesForCancel)

        { routeHdr.setAddressValue(route, false); }

        cancel.setHeader(routeHdr);
        }

        There are two problems with this solution.
        First the routes are added in reversed sequence. Since one is a stack and the
        other a list.
        I.e., if two routes were pushed in turn, say R1 and R2, then the routes header
        of the original request would be Route: R2, R1
        BUt the list would be R1, R2 and the way the route header is reconstructed would
        also be Route: R1, R2

        The second problem is that any routes already present in the cancel request
        (when creating the cancelimpl) are overwritten.

        The suggested (and tested) solution is somewhat simpler.

        if (routesForCancel != null && !routesForCancel.isEmpty()) {
        for (Address route : routesForCancel)

        { cancel.pushRoute(route); }

        }

        Show
        erikvandervelden added a comment - THe solution implemented in the end (see above) does not solve the original problem completely. The solution exists of storing all the pushed routes in a list. Then using the list to create route header in the cancel. if (routesForCancel != null && !routesForCancel.isEmpty()) { MultiLineHeader routeHdr = new MultiLineHeader(Header.ROUTE, true); for (Address route : routesForCancel) { routeHdr.setAddressValue(route, false); } cancel.setHeader(routeHdr); } There are two problems with this solution. First the routes are added in reversed sequence. Since one is a stack and the other a list. I.e., if two routes were pushed in turn, say R1 and R2, then the routes header of the original request would be Route: R2, R1 BUt the list would be R1, R2 and the way the route header is reconstructed would also be Route: R1, R2 The second problem is that any routes already present in the cancel request (when creating the cancelimpl) are overwritten. The suggested (and tested) solution is somewhat simpler. if (routesForCancel != null && !routesForCancel.isEmpty()) { for (Address route : routesForCancel) { cancel.pushRoute(route); } }
        Hide
        erikvandervelden added a comment -

        Correction;
        the suggested solution does not work since pushing routes on a cancel is not
        allowed. So the ammended solution now is this.

        • Reuse the route header is already present.
        • Insert the headers at the first position instead of the last.

        private SipServletRequestImpl createCancel() {
        SipServletRequestImpl cancel = this._request.createCancelImpl();
        if ((this.routesForCancel Unable to render embedded object: File (= null) && () not found.(this.routesForCancel.isEmpty()))) {
        // EVDV: – fixed problem that 1) routes were reversed 2) original routes not
        pushed by AR were lost
        Header routeHdr = cancel.getRawHeader("Route");
        if (routeHdr == null)

        { routeHdr = new MultiLineHeader("Route", true); cancel.setHeader(routeHdr); }

        for (Address route : this.routesForCancel)

        { routeHdr.setAddressValue(route, true); }

        }

        Show
        erikvandervelden added a comment - Correction; the suggested solution does not work since pushing routes on a cancel is not allowed. So the ammended solution now is this. Reuse the route header is already present. Insert the headers at the first position instead of the last. private SipServletRequestImpl createCancel() { SipServletRequestImpl cancel = this._request.createCancelImpl(); if ((this.routesForCancel Unable to render embedded object: File (= null) && () not found. (this.routesForCancel.isEmpty()))) { // EVDV: – fixed problem that 1) routes were reversed 2) original routes not pushed by AR were lost Header routeHdr = cancel.getRawHeader("Route"); if (routeHdr == null) { routeHdr = new MultiLineHeader("Route", true); cancel.setHeader(routeHdr); } for (Address route : this.routesForCancel) { routeHdr.setAddressValue(route, true); } }

          People

          • Assignee:
            binod
            Reporter:
            erikvandervelden
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated: