Axis2
  1. Axis2
  2. AXIS2-4465

TransportInDescription and TransportListener instances for AxisServlet are not managed properly

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5
    • Fix Version/s: 1.5.3, 1.6.0
    • Component/s: transports
    • Labels:
      None

      Description

      There are three different types of TransportListener implementations (and TransportInDescription instances) that are used during the lifecycle of AxisServlet:

      1) The default axis2.xml that we ship with the Web app has a transportReceiver entry for org.apache.axis2.transport.http.SimpleHTTPServer. This is of course wrong because we want to use AxisServlet, not the standalone HTTP server. Since AxisServlet replaces this TransportInDescription anyway (see point 2), the SimpleHTTPServer is never started (fortunately!), but it is still necessary to have the dependencies of SimpleHTTPServer in the classpath (see AXIS2-4464). Note that simply removing the transportReceiver entry from axis2.xml doesn't work, because this leads to the issue described in AXIS2-4141.

      2) AxisServlet#init creates a fresh TransportInDescription (with AxisServlet as TransportListener) and registers it in the ListenerManager/AxisConfiguration. This replaces the TransportInDescription from point 1. AxisServlet, in conjunction with ListingAgent#extractHostAndPort also implements some sort of autodetection mechanisms for the port number (using the RUNNING_PORT configuration context property). Note that all this is only done for HTTP, not HTTPS.

      3) ListingAgent can also create and register TransportInDescription instances on the fly if it detects requests using a protocol for which no TransportInDescription has been registered, i.e. HTTPS. See the addTransportListener method. This TransportInDescription instance will have yet another type of TransportListener associated with it, namely org.apache.axis2.transport.http.CustomListener.

      This approach has several problems:

      • It is a complete mess.
      • Nobody will ever be able to document this properly or explain to a user how this works.
      • It causes lots of issues. See the JIRAs linked to this report.
      • It doesn't take into account that fundamentally, no port autodetection scheme will ever work reliably (i.e. in such a way that it produces the correct WSDL from the start) if the servlet is used with both HTTP and HTTPS. The reason is that autodetection can only rely on information from incoming requests. This means that the WSDL can only be complete if the servlet has received at least on HTTP request and at least one HTTPS request.

      Here is a simple proposal to solve these problems once and for all:

      • Use a single TransportListener implementation with AxisServlet. This cannot be the AxisServlet itself because there can be two TransportListener instances for the same AxisServlet instance. It would basically be CustomListener, but with a better name, e.g. AxisServletListener.
      • Require the user to declare a transportReceiver (with AxisServletListener) in axis2.xml for every protocol (http and/or https) he wants to use.
      • If a single transportReceiver is configured, the user is not required to specify the port and AxisServlet will autodetect it based on incoming requests.
      • If a transportReceiver is configured for both http and https, the user is required to provide information about the port numbers. This makes sure that the correct WSDL can always be generated.
      • For consistency, if AxisServlet receives a request with a protocol for which no transportReceiver is configured, the request is rejected.
      • Update the default axis2.xml for the WAR distribution so that it contains: <transportReceiver name="http" class="org.apache.axis2.transport.http.AxisServletListener"/>

      The only cases where this approach may fail to give consistent results is when

      • port autodetection takes places and the WSDL is retrieved through a non HTTP protocol (very unlikely, unless something like MEX over SOAP/JMS is used...);
      • port autodetection takes places and the servlet container accepts connections for the same protocol on different ports.
        In both cases it is sufficient to specify the port number in the transportReceiver element to avoid the issue.

        Issue Links

          Activity

          Hide
          Andreas Veithen added a comment -

          The assumption you are making is that the port is only needed when a WSDL is requested using a "?wsdl" GET request. That is incorrect. There are at least two other cases where an EPR needs to be calculated:

          • MEX (which is also about WSDL generation, but which uses POST).
          • Calculating the WS-A ReplyTo header for asynchronous requests.
          Show
          Andreas Veithen added a comment - The assumption you are making is that the port is only needed when a WSDL is requested using a "?wsdl" GET request. That is incorrect. There are at least two other cases where an EPR needs to be calculated: MEX (which is also about WSDL generation, but which uses POST). Calculating the WS-A ReplyTo header for asynchronous requests.
          Hide
          nadir amra added a comment -

          OK I found the problem I was having. Currently, if we need to do auto-detection on the port we set it on any request the comes in. Basically the code is as follows:

          if (listner.getPort() == -1)

          { listner.setPort(req.getServerPort()); }

          However, assuming the port is basically used for endpoint generation in WSDLs (please correct me if I am wrong), would it be a problem if it was changed as follows:

          if (listner.getPort() == -1 && req.getMethod().equalsIgnoreCase("GET")) {
          String queryString = req.getQueryString().toUpperCase();
          if (queryString.equals("WSDL") || queryString.equals("WSDL2"))

          { listner.setPort(req.getServerPort()); }


          }

          Show
          nadir amra added a comment - OK I found the problem I was having. Currently, if we need to do auto-detection on the port we set it on any request the comes in. Basically the code is as follows: if (listner.getPort() == -1) { listner.setPort(req.getServerPort()); } However, assuming the port is basically used for endpoint generation in WSDLs (please correct me if I am wrong), would it be a problem if it was changed as follows: if (listner.getPort() == -1 && req.getMethod().equalsIgnoreCase("GET")) { String queryString = req.getQueryString().toUpperCase(); if (queryString.equals("WSDL") || queryString.equals("WSDL2")) { listner.setPort(req.getServerPort()); } }
          Hide
          Andreas Veithen added a comment -

          It will take whatever is reported by the servlet container in the HttpServletRequest. I think that that has not changed with respect to 1.5.2.

          Show
          Andreas Veithen added a comment - It will take whatever is reported by the servlet container in the HttpServletRequest. I think that that has not changed with respect to 1.5.2.
          Hide
          nadir amra added a comment -

          I have read the referenced URL.

          The question I have is whether this auto-detection takes the port from the client request (i.e. browser) or a forwarded request (i.e. an http server front-ending an application server).

          I am still investigating....

          Show
          nadir amra added a comment - I have read the referenced URL. The question I have is whether this auto-detection takes the port from the client request (i.e. browser) or a forwarded request (i.e. an http server front-ending an application server). I am still investigating....
          Hide
          Andreas Veithen added a comment -

          I don't understand the question. As explained in [1], the current implementation still supports auto-detection of port numbers.

          [1] http://axis.apache.org/axis2/java/core/docs/servlet-transport.html

          Show
          Andreas Veithen added a comment - I don't understand the question. As explained in [1] , the current implementation still supports auto-detection of port numbers. [1] http://axis.apache.org/axis2/java/core/docs/servlet-transport.html
          Hide
          nadir amra added a comment -

          Andreas,

          Regarding your comment "....also implements some sort of autodetection mechanisms for the port number (using the RUNNING_PORT configuration context property). Note that all this is only done for HTTP, not HTTPS. "

          I understand your points and they are valid in every way. Things are much better now. But just to get an idea whether this can be done or not....

          What I liked about the RUNNING_PORT support, even with its limitations, is that the port would be set to whatever the WSDL request URL automagically. Thus, if one had an HTTP server front-ending an application server hosting Axis2, and WSDL request came used the HTTP server port, then the http server port would be the port used in WSDL (assuming modifyUserWSDLPortAddress is set to true). Similarly, if WSDL request URL came to application server hosting Axis2, then the port of the application server would be used in WSDL.

          This was nice in that a change in the HTTP server port did not require a change in any configuration files.

          It seems to me that the WSDL request URL port can still be used assuming assuming that a single transportReceiver is configured and a port was not specified.

          Does this make sense to implement? Am I way off base in my analysis?

          Show
          nadir amra added a comment - Andreas, Regarding your comment "....also implements some sort of autodetection mechanisms for the port number (using the RUNNING_PORT configuration context property). Note that all this is only done for HTTP, not HTTPS. " I understand your points and they are valid in every way. Things are much better now. But just to get an idea whether this can be done or not.... What I liked about the RUNNING_PORT support, even with its limitations, is that the port would be set to whatever the WSDL request URL automagically. Thus, if one had an HTTP server front-ending an application server hosting Axis2, and WSDL request came used the HTTP server port, then the http server port would be the port used in WSDL (assuming modifyUserWSDLPortAddress is set to true). Similarly, if WSDL request URL came to application server hosting Axis2, then the port of the application server would be used in WSDL. This was nice in that a change in the HTTP server port did not require a change in any configuration files. It seems to me that the WSDL request URL port can still be used assuming assuming that a single transportReceiver is configured and a port was not specified. Does this make sense to implement? Am I way off base in my analysis?
          Hide
          Andreas Veithen added a comment -
          • The default axis2.xml bundled with the webapp was updated by Amila in r821689.
          • Documentation has been updated in r906899.
          Show
          Andreas Veithen added a comment - The default axis2.xml bundled with the webapp was updated by Amila in r821689. Documentation has been updated in r906899.
          Hide
          Andreas Veithen added a comment -

          I'm not saying that extending AxisServlet is not appropriate in general. It depends on how it is done. There are two things that are bad:

          • Making members protected (instead of private), so that the internals of AxisServlet are exposed.
          • Overriding by copy & paste, i.e. override a method of AxisServlet in a subclass by copying the original code and modifying it.

          This is bad because it means that if we don't want to break existing extensions, we are not allowed to make any changes in AxisServlet (with the exception of very localized changes).

          We are using exactly this pattern in Synapse, but I think now it is time to implement a proper solution.

          Show
          Andreas Veithen added a comment - I'm not saying that extending AxisServlet is not appropriate in general. It depends on how it is done. There are two things that are bad: Making members protected (instead of private), so that the internals of AxisServlet are exposed. Overriding by copy & paste, i.e. override a method of AxisServlet in a subclass by copying the original code and modifying it. This is bad because it means that if we don't want to break existing extensions, we are not allowed to make any changes in AxisServlet (with the exception of very localized changes). We are using exactly this pattern in Synapse, but I think now it is time to implement a proper solution.
          Hide
          Hiranya Jayathilaka added a comment -

          We don't override the initContextRoot. We use it as it is. Our custom servlet implementation extends AxisServlet and overrides a subset of its request handler methods. We need to call initContextRoot from those overridden methods.

          I agree with you on the point on firmly defining extension points. May be we need to properly document all Axis2 extension points to avoid any confusion in the future too. But I still believe that public API changes at all levels should be carried out with more care.

          Also could you please explain why you think it's not appropriate to allow AxisServlet to be extended?

          Show
          Hiranya Jayathilaka added a comment - We don't override the initContextRoot. We use it as it is. Our custom servlet implementation extends AxisServlet and overrides a subset of its request handler methods. We need to call initContextRoot from those overridden methods. I agree with you on the point on firmly defining extension points. May be we need to properly document all Axis2 extension points to avoid any confusion in the future too. But I still believe that public API changes at all levels should be carried out with more care. Also could you please explain why you think it's not appropriate to allow AxisServlet to be extended?
          Hide
          Andreas Veithen added a comment -

          If I remember well, there have been other requests in the past to make various members of AxisServlet protected so that they can be overridden/accessed by custom extensions. I don't think that is a good approach. Maybe it's time to define a set of extension points and clearly document them. Can you explain how you override or access initContextRoot?

          Show
          Andreas Veithen added a comment - If I remember well, there have been other requests in the past to make various members of AxisServlet protected so that they can be overridden/accessed by custom extensions. I don't think that is a good approach. Maybe it's time to define a set of extension points and clearly document them. Can you explain how you override or access initContextRoot?
          Hide
          Hiranya Jayathilaka added a comment -

          Hmmm....a second look at the code makes me think that simply adding the initContextRoot method back won't solve the problem completely. The two new class members (httpListener and httpsListener) are private and their initialization is handled by the AxisServlet#init method. So custom extensions which have their own init method cannot properly initialize these two members.

          Show
          Hiranya Jayathilaka added a comment - Hmmm....a second look at the code makes me think that simply adding the initContextRoot method back won't solve the problem completely. The two new class members (httpListener and httpsListener) are private and their initialization is handled by the AxisServlet#init method. So custom extensions which have their own init method cannot properly initialize these two members.
          Hide
          Hiranya Jayathilaka added a comment -

          Hi Andreas,

          It seems you have renamed one of the public methods (initContextRoot method) in the AxisServlet class and made it a private method in the process of implementing your proposal. While I totally agree with you on the solution proposed here I think it's best to not make such API changes without clearly notifying the community. As a result of this API change some of the custom extensions of the AxisServlet class that we have developed are now broken.

          So can we please refactor the AxisServlet class to negate any API changes that have been made. Looking at the code I feel this can be done easily. We can add the initContextRoot method again (as a public or protected method) and get the newly added preprocessRequest method to call it.

          WDYT?

          Show
          Hiranya Jayathilaka added a comment - Hi Andreas, It seems you have renamed one of the public methods (initContextRoot method) in the AxisServlet class and made it a private method in the process of implementing your proposal. While I totally agree with you on the solution proposed here I think it's best to not make such API changes without clearly notifying the community. As a result of this API change some of the custom extensions of the AxisServlet class that we have developed are now broken. So can we please refactor the AxisServlet class to negate any API changes that have been made. Looking at the code I feel this can be done easily. We can add the initContextRoot method again (as a public or protected method) and get the newly added preprocessRequest method to call it. WDYT?
          Hide
          Andreas Veithen added a comment -

          Implemented the change as described above. Still to do: change the default axis2.xml bundled with the webapp and update documentation.

          Show
          Andreas Veithen added a comment - Implemented the change as described above. Still to do: change the default axis2.xml bundled with the webapp and update documentation.

            People

            • Assignee:
              Andreas Veithen
              Reporter:
              Andreas Veithen
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development