CXF
  1. CXF
  2. CXF-4154

AbstractConduitSelector reused cached conduit even if protocol was changed

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.6
    • Component/s: Core
    • Labels:
      None
    • Estimated Complexity:
      Unknown

      Description

      Hi,

      Actually AbstractConduitSelector.getSelectedConduit() creates and caches conduit in selectedConduit variable. Cached conduit is reused by the next calls.

      I see the following problem: if user changed protocol in URI between calls, AbstractConduitSelector still uses cached Conduit even it cannot process new URL. In my case cached HTTP consuit tries to process UDP URL.

      Proposal for fix: check if protocol in URL was changed and if yes, close and reset selectedConduit.

      Patch is attached.

      Regards,
      Andrei.

        Activity

        Hide
        Andrei Shakirin added a comment -

        Patch

        Show
        Andrei Shakirin added a comment - Patch
        Hide
        Daniel Kulp added a comment -

        There is a threading problem with the proposal that I definitely need to think more about. If two threads are making requests, one of which changes the protocol, it can close the conduit used by the other thread prematurely.

        Show
        Daniel Kulp added a comment - There is a threading problem with the proposal that I definitely need to think more about. If two threads are making requests, one of which changes the protocol, it can close the conduit used by the other thread prematurely.
        Hide
        Andrei Shakirin added a comment -

        Yep, you are right. Case is a little bit exotic, but anyway - patch is not safe.
        I am going to fix it in the following way: provide a thread safe cache map for the conduits with protocol as key. In getSelectedConduit() check map first and create new conduit only if it is not cached for requested protocol. In close() release all conduits from the map. Patch will be improved soon.

        Show
        Andrei Shakirin added a comment - Yep, you are right. Case is a little bit exotic, but anyway - patch is not safe. I am going to fix it in the following way: provide a thread safe cache map for the conduits with protocol as key. In getSelectedConduit() check map first and create new conduit only if it is not cached for requested protocol. In close() release all conduits from the map. Patch will be improved soon.
        Hide
        Daniel Kulp added a comment -

        I'm testing a different fix today. Hopefully will commit on monday.

        Show
        Daniel Kulp added a comment - I'm testing a different fix today. Hopefully will commit on monday.
        Hide
        Andrei Shakirin added a comment -

        Ok, very curious to see how you fixed it.

        Show
        Andrei Shakirin added a comment - Ok, very curious to see how you fixed it.
        Hide
        Daniel Kulp added a comment -

        Committed a fix last night that pretty much removes the "selectedConduit" field and keeps a list of conduits. Under normal operation, that's a single conduit, but it can "grow" as additional protocols and such require it.

        Because of the field removal, not back portable to <=2.5.x. Will need to be release noted.

        Can you give it a quick spin to make sure it works OK?

        Show
        Daniel Kulp added a comment - Committed a fix last night that pretty much removes the "selectedConduit" field and keeps a list of conduits. Under normal operation, that's a single conduit, but it can "grow" as additional protocols and such require it. Because of the field removal, not back portable to <=2.5.x. Will need to be release noted. Can you give it a quick spin to make sure it works OK?
        Hide
        Andrei Shakirin added a comment - - edited

        Just tested with my use case - your fix works!

        Only snapshoot in findCompatibleConduit() I did not completely understand:

                for (Conduit c2 : conduits) {
                    if (c2.getTarget() == null 
                        || c2.getTarget().getAddress() == null
                        || c2.getTarget().getAddress().getValue() == null) {
                        return c2;
                    }
                }
        

        Does conduit with not initialized target endpoint make sense at all? AbstractConduit constructor requires target endpoint argument, of course it is possible to give null there - but is there any use case for it?
        Do you think it is legal to choose such conduit if there is no another one that fit better (regarding protocol or full URL)?
        I would propose just return null in findCompatibleConduit() for this case and re-initialse conduit.

        Show
        Andrei Shakirin added a comment - - edited Just tested with my use case - your fix works! Only snapshoot in findCompatibleConduit() I did not completely understand: for (Conduit c2 : conduits) { if (c2.getTarget() == null || c2.getTarget().getAddress() == null || c2.getTarget().getAddress().getValue() == null ) { return c2; } } Does conduit with not initialized target endpoint make sense at all? AbstractConduit constructor requires target endpoint argument, of course it is possible to give null there - but is there any use case for it? Do you think it is legal to choose such conduit if there is no another one that fit better (regarding protocol or full URL)? I would propose just return null in findCompatibleConduit() for this case and re-initialse conduit.
        Hide
        Daniel Kulp added a comment -

        It is a valid use case. Using the UpfrontConduitSelector, an application could configure in a conduit that "protocol less" or similar that is not specific to a target. As an example, an ESB, instead of creating a conduit for each protocol, could just configure in a "SuperDoEverythingConduit" singleton or similar that is not target specific. We do want to allow that type of thing. (and there is a test that hits it which is why I added that check)

        Anyway, thanks for verifying that the fix works.

        Show
        Daniel Kulp added a comment - It is a valid use case. Using the UpfrontConduitSelector, an application could configure in a conduit that "protocol less" or similar that is not specific to a target. As an example, an ESB, instead of creating a conduit for each protocol, could just configure in a "SuperDoEverythingConduit" singleton or similar that is not target specific. We do want to allow that type of thing. (and there is a test that hits it which is why I added that check) Anyway, thanks for verifying that the fix works.

          People

          • Assignee:
            Daniel Kulp
            Reporter:
            Andrei Shakirin
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development