Axis2
  1. Axis2
  2. AXIS2-4751

Cookie value is always kept whatever SESSION_MAINTAIN_PROPERTY value is configured

    Details

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

      Description

      Cookie value is always kept whatever SESSION_MAINTAIN_PROPERTY value is configured

        Activity

        Hide
        Ivan added a comment -

        Attach a patch, try to clear the cookie values if session_maintain_property is false.

        Show
        Ivan added a comment - Attach a patch, try to clear the cookie values if session_maintain_property is false.
        Hide
        Jarek Gawor added a comment -

        Ivan,

        I believe the problem is that the HttpClient instance is being reused here between calls and call-specific things like HttpParams and HttpState are being shared. Since the HttpClient instance is shared, resetting the cookies on the shared HttpState might have bad consequences for other clients executing at the same time.

        So, I updated the code not to reuse HttpClient instances. The HttpClient instances will share the a single HttpConnectionManager instance which should result in proper connection management.

        Please test this out and let me know if everything still works ok.

        Show
        Jarek Gawor added a comment - Ivan, I believe the problem is that the HttpClient instance is being reused here between calls and call-specific things like HttpParams and HttpState are being shared. Since the HttpClient instance is shared, resetting the cookies on the shared HttpState might have bad consequences for other clients executing at the same time. So, I updated the code not to reuse HttpClient instances. The HttpClient instances will share the a single HttpConnectionManager instance which should result in proper connection management. Please test this out and let me know if everything still works ok.
        Hide
        Glen Daniels added a comment -

        Jarek,

        Please see http://hc.apache.org/httpclient-3.x/threading.html and http://hc.apache.org/httpclient-3.x/performance.html, both of which pretty clearly recommend sharing a single HTTPClient instance as widely as possible, which is part of why I implemented that solution for AXIS2-935, AXIS2-2883, etc.

        I think the right solution here is to arrange for HttpState objects to be kept with the individual ServiceClients or ConfigurationContexts (depending on the scope of session management you're looking for) and passed down into the individual httpClient.executeMethod() calls at the end of AbstractHTTPSender.executeMethod().

        The change you made doesn't seem to break my little test (http://www.mail-archive.com/axis-dev@ws.apache.org/msg47633.html), but I think I'm still uncomfortable with it based on earlier research...

        Thoughts?

        Show
        Glen Daniels added a comment - Jarek, Please see http://hc.apache.org/httpclient-3.x/threading.html and http://hc.apache.org/httpclient-3.x/performance.html , both of which pretty clearly recommend sharing a single HTTPClient instance as widely as possible, which is part of why I implemented that solution for AXIS2-935 , AXIS2-2883 , etc. I think the right solution here is to arrange for HttpState objects to be kept with the individual ServiceClients or ConfigurationContexts (depending on the scope of session management you're looking for) and passed down into the individual httpClient.executeMethod() calls at the end of AbstractHTTPSender.executeMethod(). The change you made doesn't seem to break my little test ( http://www.mail-archive.com/axis-dev@ws.apache.org/msg47633.html ), but I think I'm still uncomfortable with it based on earlier research... Thoughts?
        Hide
        Jarek Gawor added a comment -

        Glen,

        I read those pages before and I think they are misleading. If all requests use the same configuration then yes reusing HttpClient instance makes sense. But if each request can have different settings (e.g. HTTP 1.0 vs. HTTP 1.1, different socket timeouts, cookies, etc.) then reusing HttpClient does not work. And that's what is done in the AbstractHTTPSender. Anytime there is a call to httpClient.getParams() to set some option that is a potential problem from thread safety point of view since we can't know what actual parameters will be used for a given call.
        To deal with HttpState, yes we can pass a new instance of it on each httpClient.executeMethod() but that still won't address the HttpParams issue.
        There is nothing special about HttpClient. It doesn't do any special multi-threaded connection management. It's just a simple wrapper that makes making multiple http calls using the same configuration a bit easier. Just take a look at the source code: http://svn.apache.org/viewvc/httpcomponents/oac.hc3x/tags/HTTPCLIENT_3_1/src/java/org/apache/commons/httpclient/HttpClient.java?view=markup

        From my POV creating new HttpClient instances is a little bit easier since we don't have to worry about thread-safety at all. But if we want to reuse HttpClient instances we have to make sure (by rewriting some code) that setting one parameter on one thread does not affect a request on another thread.

        Show
        Jarek Gawor added a comment - Glen, I read those pages before and I think they are misleading. If all requests use the same configuration then yes reusing HttpClient instance makes sense. But if each request can have different settings (e.g. HTTP 1.0 vs. HTTP 1.1, different socket timeouts, cookies, etc.) then reusing HttpClient does not work. And that's what is done in the AbstractHTTPSender. Anytime there is a call to httpClient.getParams() to set some option that is a potential problem from thread safety point of view since we can't know what actual parameters will be used for a given call. To deal with HttpState, yes we can pass a new instance of it on each httpClient.executeMethod() but that still won't address the HttpParams issue. There is nothing special about HttpClient. It doesn't do any special multi-threaded connection management. It's just a simple wrapper that makes making multiple http calls using the same configuration a bit easier. Just take a look at the source code: http://svn.apache.org/viewvc/httpcomponents/oac.hc3x/tags/HTTPCLIENT_3_1/src/java/org/apache/commons/httpclient/HttpClient.java?view=markup From my POV creating new HttpClient instances is a little bit easier since we don't have to worry about thread-safety at all. But if we want to reuse HttpClient instances we have to make sure (by rewriting some code) that setting one parameter on one thread does not affect a request on another thread.
        Hide
        Andreas Veithen added a comment -

        I had to revert r958696 (and r958718 on the 1.5 branch) because this change causes a regression in Rampart. The change causes a failure in org.apache.rampart.RampartTest:

        Unable to sendViaPost to urlhttp://127.0.0.1:5555/axis2/services/SecureServiceSC1
        org.apache.commons.httpclient.ConnectionPoolTimeoutException: Timeout waiting for connection
        at org.apache.commons.httpclient.MultiThreadedHttpConnectionManager.doGetConnection(MultiThreadedHttpConnectionManager.java:497)
        at org.apache.commons.httpclient.MultiThreadedHttpConnectionManager.getConnectionWithTimeout(MultiThreadedHttpConnectionManager.java:416)
        at org.apache.commons.httpclient.HttpMethodDirector.executeMethod(HttpMethodDirector.java:153)
        at org.apache.commons.httpclient.HttpClient.executeMethod(HttpClient.java:397)
        at org.apache.axis2.transport.http.AbstractHTTPSender.executeMethod(AbstractHTTPSender.java:567)
        at org.apache.axis2.transport.http.HTTPSender.sendViaPost(HTTPSender.java:193)
        at org.apache.axis2.transport.http.HTTPSender.send(HTTPSender.java:75)
        at org.apache.axis2.transport.http.CommonsHTTPTransportSender.writeMessageWithCommons(CommonsHTTPTransportSender.java:415)
        at org.apache.axis2.transport.http.CommonsHTTPTransportSender.invoke(CommonsHTTPTransportSender.java:240)
        at org.apache.axis2.engine.AxisEngine.send(AxisEngine.java:443)
        at org.apache.axis2.description.OutInAxisOperationClient.send(OutInAxisOperation.java:406)
        at org.apache.axis2.description.OutInAxisOperationClient.executeImpl(OutInAxisOperation.java:229)
        at org.apache.axis2.client.OperationClient.execute(OperationClient.java:165)
        at org.apache.axis2.client.ServiceClient.sendReceive(ServiceClient.java:555)
        at org.apache.axis2.client.ServiceClient.sendReceive(ServiceClient.java:531)
        at org.apache.rampart.RampartTest.testWithPolicy(RampartTest.java:185)

        Show
        Andreas Veithen added a comment - I had to revert r958696 (and r958718 on the 1.5 branch) because this change causes a regression in Rampart. The change causes a failure in org.apache.rampart.RampartTest: Unable to sendViaPost to url http://127.0.0.1:5555/axis2/services/SecureServiceSC1 org.apache.commons.httpclient.ConnectionPoolTimeoutException: Timeout waiting for connection at org.apache.commons.httpclient.MultiThreadedHttpConnectionManager.doGetConnection(MultiThreadedHttpConnectionManager.java:497) at org.apache.commons.httpclient.MultiThreadedHttpConnectionManager.getConnectionWithTimeout(MultiThreadedHttpConnectionManager.java:416) at org.apache.commons.httpclient.HttpMethodDirector.executeMethod(HttpMethodDirector.java:153) at org.apache.commons.httpclient.HttpClient.executeMethod(HttpClient.java:397) at org.apache.axis2.transport.http.AbstractHTTPSender.executeMethod(AbstractHTTPSender.java:567) at org.apache.axis2.transport.http.HTTPSender.sendViaPost(HTTPSender.java:193) at org.apache.axis2.transport.http.HTTPSender.send(HTTPSender.java:75) at org.apache.axis2.transport.http.CommonsHTTPTransportSender.writeMessageWithCommons(CommonsHTTPTransportSender.java:415) at org.apache.axis2.transport.http.CommonsHTTPTransportSender.invoke(CommonsHTTPTransportSender.java:240) at org.apache.axis2.engine.AxisEngine.send(AxisEngine.java:443) at org.apache.axis2.description.OutInAxisOperationClient.send(OutInAxisOperation.java:406) at org.apache.axis2.description.OutInAxisOperationClient.executeImpl(OutInAxisOperation.java:229) at org.apache.axis2.client.OperationClient.execute(OperationClient.java:165) at org.apache.axis2.client.ServiceClient.sendReceive(ServiceClient.java:555) at org.apache.axis2.client.ServiceClient.sendReceive(ServiceClient.java:531) at org.apache.rampart.RampartTest.testWithPolicy(RampartTest.java:185)
        Hide
        Shawn Jiang added a comment -

        Hi Andreas,

        I prefer to keep Jarek's change because this JIRA is causing a tck error.

        Instead of reverting the fix for this JIRA. Could you please try ServiceClient.cleanup() or ServiceClient.cleanupTransport to see if they helps in RampartTest ?

        Show
        Shawn Jiang added a comment - Hi Andreas, I prefer to keep Jarek's change because this JIRA is causing a tck error. Instead of reverting the fix for this JIRA. Could you please try ServiceClient.cleanup() or ServiceClient.cleanupTransport to see if they helps in RampartTest ?
        Hide
        Andreas Veithen added a comment -

        I forgot to give the following additional info:

        • I was initially assuming that this is indeed caused by an issue in RampartTest. I added calls to ServiceClient#cleanupTransport, but that didn't change anything. Note that I didn't try ServiceClient#cleanup, but the Javadoc clearly says that ServiceClient#cleanupTransport is the right method to call in this case.
        • The change causes even more massive issues in the Sandesha2 build.
        Show
        Andreas Veithen added a comment - I forgot to give the following additional info: I was initially assuming that this is indeed caused by an issue in RampartTest. I added calls to ServiceClient#cleanupTransport, but that didn't change anything. Note that I didn't try ServiceClient#cleanup, but the Javadoc clearly says that ServiceClient#cleanupTransport is the right method to call in this case. The change causes even more massive issues in the Sandesha2 build.
        Hide
        Jarek Gawor added a comment -

        I recommitted my changes to trunk (revision 963710) to 1.5 branch (revision 963711) with a small change that actually disables automatic connection reuse. It seems there are other bugs in Axis2 or related libraries or tests that cause the test failures when the connection manager is properly reused.

        But let me start over. First of all, before my changes the getHttpClient() code in trunk and 1.5 branch were different. The code in 1.5 branch was 'newer' than in trunk and was attempting to automatically reuse the HttpClient/HttpConnectionManager. The code in trunk would not automatically reuse the HttpClient/HttpConnectionManager. I guess the code from 1.5 didn't get merged to trunk at some point.
        If the code from 1.5 branch was merged to trunk we would see the same type of failures in Rampart, etc. as with my changes. So there are some other bugs in Axis2 or related libraries or badly written tests that cause these problems when connection reuse is enabled.

        On top of that the code in 1.5 branch was attempting to reuse the HttpClient instance. And as mentioned before the way the HttpClient is used in the AbstractHTTPSender.java is not thread safe. So a single HttpClient instance cannot be reused unless we update some code & apis.

        As to the test failures, the tests fail because the connection pool runs out of available connections. That most likely indicates that some connections are not released correctly. The connections should be released automatically if the input stream is completely consumed or TransportSender.cleanup() is called. I think some tests are either not fully consuming the input or releasing the connections. For example, the RampartTest passes for me if I add options.setCallTransportCleanup(true); to RampartUtil.java:751.

        Show
        Jarek Gawor added a comment - I recommitted my changes to trunk (revision 963710) to 1.5 branch (revision 963711) with a small change that actually disables automatic connection reuse. It seems there are other bugs in Axis2 or related libraries or tests that cause the test failures when the connection manager is properly reused. But let me start over. First of all, before my changes the getHttpClient() code in trunk and 1.5 branch were different. The code in 1.5 branch was 'newer' than in trunk and was attempting to automatically reuse the HttpClient/HttpConnectionManager. The code in trunk would not automatically reuse the HttpClient/HttpConnectionManager. I guess the code from 1.5 didn't get merged to trunk at some point. If the code from 1.5 branch was merged to trunk we would see the same type of failures in Rampart, etc. as with my changes. So there are some other bugs in Axis2 or related libraries or badly written tests that cause these problems when connection reuse is enabled. On top of that the code in 1.5 branch was attempting to reuse the HttpClient instance. And as mentioned before the way the HttpClient is used in the AbstractHTTPSender.java is not thread safe. So a single HttpClient instance cannot be reused unless we update some code & apis. As to the test failures, the tests fail because the connection pool runs out of available connections. That most likely indicates that some connections are not released correctly. The connections should be released automatically if the input stream is completely consumed or TransportSender.cleanup() is called. I think some tests are either not fully consuming the input or releasing the connections. For example, the RampartTest passes for me if I add options.setCallTransportCleanup(true); to RampartUtil.java:751.
        Hide
        Glen Daniels added a comment -

        Hi Jarek:

        Unfortunately I don't have time to re-dive into this just now, but hope to in the next couple of days. I know that the 1.5.1 version did work with both Rampart and Sandesha at the time, I tested with both before releasing.

        I'd like to get this right "once and for all", and make sure we have tests to keep it from breaking again.

        Show
        Glen Daniels added a comment - Hi Jarek: Unfortunately I don't have time to re-dive into this just now, but hope to in the next couple of days. I know that the 1.5.1 version did work with both Rampart and Sandesha at the time, I tested with both before releasing. I'd like to get this right "once and for all", and make sure we have tests to keep it from breaking again.
        Hide
        Andreas Veithen added a comment -

        For the moment, the Hudson build fails because of another change. I will comment on this issue once the Axis2 build is stable again and we can see the impact that the change has on downstream projects.

        Show
        Andreas Veithen added a comment - For the moment, the Hudson build fails because of another change. I will comment on this issue once the Axis2 build is stable again and we can see the impact that the change has on downstream projects.
        Hide
        Andreas Veithen added a comment -

        All downstream builds are successful now. Once Glen has reviewed the change, we may close the issue.

        Show
        Andreas Veithen added a comment - All downstream builds are successful now. Once Glen has reviewed the change, we may close the issue.
        Hide
        Andreas Veithen added a comment -

        I think there were some comments from Amila related to this issue. Have they been taken into account? If so, can we close the issue?

        Show
        Andreas Veithen added a comment - I think there were some comments from Amila related to this issue. Have they been taken into account? If so, can we close the issue?
        Hide
        Andreas Veithen added a comment -

        I've restored r958718 on the 1.5 branch and merged that change as well as Glen's fix for the CLOSE_WAIT issue (which was only done on the 1.5 branch for the 1.5.1 release) back to the trunk. I also fixed an issue in Rampart's STSClient and the Rampart test all pass now.

        Show
        Andreas Veithen added a comment - I've restored r958718 on the 1.5 branch and merged that change as well as Glen's fix for the CLOSE_WAIT issue (which was only done on the 1.5 branch for the 1.5.1 release) back to the trunk. I also fixed an issue in Rampart's STSClient and the Rampart test all pass now.

          People

          • Assignee:
            Jarek Gawor
            Reporter:
            Ivan
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development