CXF
  1. CXF
  2. CXF-3916

partial response problem with SOAP 1.1 use of WS-Addressing

    Details

    • Estimated Complexity:
      Moderate

      Description

      Description copied from email:

      I've read over this more and now see that the partial response stuff is definitely for asynchronous processing, so the check with the WS-Addressing relatesTo header makes sense. The problem (I think) appears in your checkin revision 705446 for ClientImpl.java in this section:

          synchronized (message.getExchange()) {
              if (!isPartialResponse(message) && callback == null) {
                  message.getExchange().put(FINISHED, Boolean.TRUE);
                  message.getExchange().setInMessage(message);
                  message.getExchange().notifyAll();                   
              }
          }
      

      You added the "&& callback == null" test, but I think what is needed is "|| callback == null". The idea here (again, as I'm reading it) is regarding these two cases:

      • it's an asynchronous response which is not a partial response
      • there is no callback, meaning it's a synchronous response

      In either of these cases you want to tell the exchange that it's finished and the message you just got is the inbound message. I think this worked for a long time without anyone running into this because in the synchronous case (callback == null), the only way you get a partialResponse==true is when WS-Addressing is engaged AND the server that you're connecting to doesn't return the optional (but almost always used) relatesTo header. Probably in the vast majority of cases either WS-Addressing isn't used or the relatesTo header is present in a response.

      If you agree, I can create a defect and describe this. Since the change is just && to ||, obviously it won't help to send you a patch file

      Thanks,
      Jesse

      -----Original Message-----
      From: Jesse Pangburn [mailto:Jesse.Pangburn@us.lawson.com] 
      Sent: Wednesday, November 09, 2011 6:37 PM
      To: users@cxf.apache.org
      Subject: partial response problem with SOAP 1.1 use of WS-Addressing and SOAPAction
      
      Hi,
      I invoked a SOAP 1.1 web service using CXF 2.4.2 DispatchImpl and that service immediately returned the following soap header:
      	<soap:Header>
      		<wsa:MessageID>uuid:A12B3727-0B3D-11E1-983D-DFB5348FF699</wsa:MessageID>
      		<wsa:Action>response</wsa:Action>
      	</soap:Header>
      
      My client hung for 60 seconds until a timeout was reached, at which point the response was available in the StaxSource.  Tracing the problem into the code revealed that it was waiting because the message response it had received so far was deemed a "partial response" due to the following code which always is called when WS-Addressing is enabled in MAPCodec.java:
          private void markPartialResponse(SoapMessage message, AddressingProperties maps) {
              if (ContextUtils.isRequestor(message) && null != maps
                  && (null == maps.getRelatesTo() 
                      || (null != maps.getRelatesTo()
                          && Names.WSA_UNSPECIFIED_RELATIONSHIP.equals(maps.getRelatesTo().getValue())))) {
                  message.put(Message.PARTIAL_RESPONSE_MESSAGE, Boolean.TRUE);
              } 
          }
      
      The problem, I think, is this condition "null == maps.getRelatesTo()".  This essentially means that a WS-Addressing RelatesTo header is required to indicate that a message response is complete- even on a synchronous request/response.  I think the source of this problem is that the original WS-Addressing submission to W3C said that "This element MUST be present if the message is a reply" in the description for the RelatesTo header (see http://www.w3.org/Submission/ws-addressing/#_Toc77464323).  This language was struck from the final WS-Addressing 1.0 (see http://www.w3.org/TR/ws-addr-core/#msgaddrpropsinfoset) and means that RelatesTo is not required.
      
      While I think it was sloppy on the part of the service writer to not include the RelatesTo header, it is OPTIONAL according to the spec.  So, especially in the case of a synchronous request, I think this code is incorrect.  A CXF Dispatch client should not hang until timeout is reached because an optional header is not included in the response.
      
      Unfortunately, I'm not really sure what the correct solution is here since I don't understand the case for ever having a partial response message in a synchronous request/response.  Should later code note that the request/response is synchronous and ignore this partial response flag?  I assume the intention of this code is for asynchronous request/response so that the immediate response on the request's socket connection is not treated as the asynchronous response message.
      
      Any clues?
      Thanks,
      Jesse
      

        Activity

        Hide
        Jesse Pangburn added a comment -

        The previous fix applied does not resolve the issue. I've tried this using 2.5.2 and the fix says it's resolved in 2.5.1, but still getting the same problem.

        When the response message comes back like this without a relatesTo header is when the trouble happens:
        <soap:Header>
        <wsa:MessageID>uuid:7F0786FA-5118-11E1-9ACE-BB6287EFAC14</wsa:MessageID>
        <wsa:Action>response</wsa:Action>
        </soap:Header>

        The original code was:
        if (!isPartialResponse(message) && callback == null)

        { message.getExchange().put(FINISHED, Boolean.TRUE); message.getExchange().setInMessage(message); message.getExchange().notifyAll(); }

        This was clearly wrong and so I think Dan changed the test to be:
        if (!isPartialResponse(message)) {

        I believe the intention is that this test would do what it implies "if the response is not a partial response then...". If that were true this would have worked. It's not true though, because that test returns true in the case of a synchronous request with no relatesTo header- saying that it's a partial response when it's not. It's the complete response because it was a synchronous response.

        The fault here really lies in the code that sets the variable this test checks, but it's hard to fix there. This is why my original suggestion was to change the test to:
        if (!isPartialResponse(message) || callback == null) {

        This fixes the synchronous case because callback is always null for a synchronous response. It does not harm the test for asynchronous case because callback is not null.

        Changing 2.5.2 code to have this test instead works with that response which is missing the relatesTo header. The response is returned immediately to the client instead of timing out waiting for the full response to come back (when there is no more forthcoming).

        thanks,
        Jesse

        Show
        Jesse Pangburn added a comment - The previous fix applied does not resolve the issue. I've tried this using 2.5.2 and the fix says it's resolved in 2.5.1, but still getting the same problem. When the response message comes back like this without a relatesTo header is when the trouble happens: <soap:Header> <wsa:MessageID>uuid:7F0786FA-5118-11E1-9ACE-BB6287EFAC14</wsa:MessageID> <wsa:Action>response</wsa:Action> </soap:Header> The original code was: if (!isPartialResponse(message) && callback == null) { message.getExchange().put(FINISHED, Boolean.TRUE); message.getExchange().setInMessage(message); message.getExchange().notifyAll(); } This was clearly wrong and so I think Dan changed the test to be: if (!isPartialResponse(message)) { I believe the intention is that this test would do what it implies "if the response is not a partial response then...". If that were true this would have worked. It's not true though, because that test returns true in the case of a synchronous request with no relatesTo header- saying that it's a partial response when it's not. It's the complete response because it was a synchronous response. The fault here really lies in the code that sets the variable this test checks, but it's hard to fix there. This is why my original suggestion was to change the test to: if (!isPartialResponse(message) || callback == null) { This fixes the synchronous case because callback is always null for a synchronous response. It does not harm the test for asynchronous case because callback is not null. Changing 2.5.2 code to have this test instead works with that response which is missing the relatesTo header. The response is returned immediately to the client instead of timing out waiting for the full response to come back (when there is no more forthcoming). thanks, Jesse
        Hide
        Daniel Kulp added a comment -

        Added a testcase for this.

        Basing on the presence or not of the callback is definitely not a correct fix and the testcase I added actually proved it. A couple interceptors based their actions on whether the message was a partial response or not and thus things were not processed correctly. For example, in the testcase, the response resulted in a ClassCastException as the WrapperClassInInterceptor does not run if the response is a partial message so the contents List had the wrapper object and not the expected integer. Also, with WS-RM, you can have partial responses and such coming in whether or not you have a callback object.

        I've updated the MAPAggregator to do some extra checks if the message is marked a partial response to reset to a non-partial response if it feels it should be. That fixes the test.

        It would be great if you could try your cases with tonights snapshots (or build your own snapshots).

        Show
        Daniel Kulp added a comment - Added a testcase for this. Basing on the presence or not of the callback is definitely not a correct fix and the testcase I added actually proved it. A couple interceptors based their actions on whether the message was a partial response or not and thus things were not processed correctly. For example, in the testcase, the response resulted in a ClassCastException as the WrapperClassInInterceptor does not run if the response is a partial message so the contents List had the wrapper object and not the expected integer. Also, with WS-RM, you can have partial responses and such coming in whether or not you have a callback object. I've updated the MAPAggregator to do some extra checks if the message is marked a partial response to reset to a non-partial response if it feels it should be. That fixes the test. It would be great if you could try your cases with tonights snapshots (or build your own snapshots).
        Hide
        Jesse Pangburn added a comment -

        Hi Dan,
        Perfect, this is definitely the best fix so that isPartialResponse() does what it says it does. I hadn't considered other downstream interceptors using isPartialResponse and having problems since I didn't encounter any problems like that with my quick fix and don't use WS-RM. Thank you for fixing it right!

        I downloaded the 2.5.3 snapshot this morning and tested it and it worked great with the endpoint that was causing me trouble.

        Thanks again,
        Jesse

        Show
        Jesse Pangburn added a comment - Hi Dan, Perfect, this is definitely the best fix so that isPartialResponse() does what it says it does. I hadn't considered other downstream interceptors using isPartialResponse and having problems since I didn't encounter any problems like that with my quick fix and don't use WS-RM. Thank you for fixing it right! I downloaded the 2.5.3 snapshot this morning and tested it and it worked great with the endpoint that was causing me trouble. Thanks again, Jesse
        Hide
        Daniel Kulp added a comment -

        Jesse,

        Aki just pointed out to me that according to the WS-Addressing spec, the wsa:RelatesTo header MUST be present if the message is a reply. I'm going to leave my fix in place, but I'm pointing this out to you as you may want to fix the service that is generating the invalid WS-Addressing response.

        Dan

        Show
        Daniel Kulp added a comment - Jesse, Aki just pointed out to me that according to the WS-Addressing spec, the wsa:RelatesTo header MUST be present if the message is a reply. I'm going to leave my fix in place, but I'm pointing this out to you as you may want to fix the service that is generating the invalid WS-Addressing response. Dan
        Hide
        Jesse Pangburn added a comment -

        Hi Dan,
        I don't think that's true. I looked up the spec before going down this path. The W3C spec is at http://www.w3.org/TR/ws-addr-core/

        Here's the part about relatesTo:
        /wsa:RelatesTo
        This OPTIONAL (repeating) element information item contributes one abstract [relationship] property value, in the form of an (IRI, IRI) pair. The content of this element (of type xs:anyURI) conveys the [message id] of the related message.

        The spec says nothing about it being required, in fact it says in caps "OPTIONAL".

        Aki is probably looking at the original W3C submission, not the final spec. The submission is available here: http://www.w3.org/Submission/ws-addressing/

        It's different. It says:
        /wsa:RelatesTo
        This OPTIONAL (repeating) element information item contributes one abstract [relationship] property value, in the form of a (URI, QName) pair. The [children] property of this element (which is of type xs:anyURI) conveys the [message id] of the related message. This element MUST be present if the message is a reply.

        This is probably where he got the idea that it was required, since it is required in the submission version. Apparently that was lifted in the final version.

        Thanks,
        Jesse

        Show
        Jesse Pangburn added a comment - Hi Dan, I don't think that's true. I looked up the spec before going down this path. The W3C spec is at http://www.w3.org/TR/ws-addr-core/ Here's the part about relatesTo: /wsa:RelatesTo This OPTIONAL (repeating) element information item contributes one abstract [relationship] property value, in the form of an (IRI, IRI) pair. The content of this element (of type xs:anyURI) conveys the [message id] of the related message. The spec says nothing about it being required, in fact it says in caps "OPTIONAL". Aki is probably looking at the original W3C submission, not the final spec. The submission is available here: http://www.w3.org/Submission/ws-addressing/ It's different. It says: /wsa:RelatesTo This OPTIONAL (repeating) element information item contributes one abstract [relationship] property value, in the form of a (URI, QName) pair. The [children] property of this element (which is of type xs:anyURI) conveys the [message id] of the related message. This element MUST be present if the message is a reply. This is probably where he got the idea that it was required, since it is required in the submission version. Apparently that was lifted in the final version. Thanks, Jesse
        Hide
        Daniel Kulp added a comment - - edited

        Right. However, it is specified in section 3.4 about creating a reply message:

        [relationship]: this property MUST include a pair of IRIs as follows; the relationship type is the predefined reply URI "http://www.w3.org/2005/08/addressing/reply" and the related message's identifier is the [message id] property value from the message being replied to; other relationships MAY be expressed in this property

        So the relationship must be specified.

        So it's optional in that it isn't on all messages, but for a message formulating a reply, it must be there to formulate the relationship.

        Show
        Daniel Kulp added a comment - - edited Right. However, it is specified in section 3.4 about creating a reply message: [relationship] : this property MUST include a pair of IRIs as follows; the relationship type is the predefined reply URI "http://www.w3.org/2005/08/addressing/reply" and the related message's identifier is the [message id] property value from the message being replied to; other relationships MAY be expressed in this property So the relationship must be specified. So it's optional in that it isn't on all messages, but for a message formulating a reply, it must be there to formulate the relationship.
        Hide
        Jesse Pangburn added a comment -

        Hi Dan,
        That makes sense. I was searching the doc for "relatesTo" and it isn't mentioned there. I see the interpretation making sense though. The service I was calling was beyond my control- it's a test service for a government service.

        Still, doesn't hurt us (CXF users) to be tolerant. Follows the send precise / receive tolerant idea.

        Thanks,
        Jesse

        Show
        Jesse Pangburn added a comment - Hi Dan, That makes sense. I was searching the doc for "relatesTo" and it isn't mentioned there. I see the interpretation making sense though. The service I was calling was beyond my control- it's a test service for a government service. Still, doesn't hurt us (CXF users) to be tolerant. Follows the send precise / receive tolerant idea. Thanks, Jesse

          People

          • Assignee:
            Daniel Kulp
            Reporter:
            Jesse Pangburn
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development