Qpid
  1. Qpid
  2. QPID-3157

'removed' subscriptions may be held in memory by the queues SubscriptionList or 'lastSubscriptionNode' reference

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 0.5, 0.6, 0.7, 0.8, 0.9, 0.10, 0.11, 0.12
    • Fix Version/s: 0.13
    • Component/s: Java Broker
    • Labels:
      None

      Description

      subscriptions 'removed' from a queue subscription list are only marked deleted and can not actually bereleased from memory until the head of the subscription list advances beyond them. Additionally and perhaps more troublesome, a queues 'lastSubscriptionNode' can refer to a particular subscription but hold all subscequent subscriptions in memory whether they have been deleted or not and regardless whether the head of the SubscriptionList has advanced beyond them,

      As a result any memory in use by the now-closed subscription will not be released until the queue is deleted, or all the subscriptions prior to it are closed and removed from the list and the 'lastSubscriptionNode' advances beyond them.. This also holds the associated ServerSession in memory, which is currently a rather heavyweight object.

      This issue is further compounded when the queue has a backlog of messages and consumers are then created, used to recieve one message, and closed. In this scenario the broker sends the client as many messages as it can prefetch, leading to creation of up to <prefetch size, default=500> MessageTransfer commands, all which are recorded in the ServerSession but then left retained as 'unprocessed' in the closed session, which might be held in memory as described above. This results in an explosion in the number of MessageTransfer commands retained in memory as each message can have up to <prefetch size> MessageTransfer commands associated with it before it is eventually consumed by a client and all of thse will also be retained in memory by a 'removed' but not deleted subscription. The last effect can be combated by restricting the preftech size (eg appending &maxprefetch='1' to the connection url).

        Issue Links

          Activity

          Hide
          Robbie Gemmell added a comment -

          Updating this with additional details from further analysis, and bumping out to 0.11: none of these issues are new to 0.10 (its been this way since time began apparently) and the number of aspects involved here make this too sensitive to introduce changes to so late in the 0.10 release process.

          Show
          Robbie Gemmell added a comment - Updating this with additional details from further analysis, and bumping out to 0.11: none of these issues are new to 0.10 (its been this way since time began apparently) and the number of aspects involved here make this too sensitive to introduce changes to so late in the 0.10 release process.
          Hide
          Robbie Gemmell added a comment -

          Hi Keith, could you review these changes please?

          The SubscriptionList now ensures it removes the nodes during the remove method rather than waiting for the list head to advance past them, which may not occur if earlier Subscription nodes in the list remain active. A dummy node is inserted at the end when attempting to remove the current tail node, as the tail cant be unlinked for thread safety. The new 'marked node' functionlity within the SubscriptionList allows cleaning up any nodes the prior 'lastSubscriptionNode' reference in the queue would have leaked by simply searching for the next non-deleted node after the marker, since this is guaranteed to be within the active list, after which point they are no longer leaked due to the above changes.

          Thanks,
          Robbie.

          Show
          Robbie Gemmell added a comment - Hi Keith, could you review these changes please? The SubscriptionList now ensures it removes the nodes during the remove method rather than waiting for the list head to advance past them, which may not occur if earlier Subscription nodes in the list remain active. A dummy node is inserted at the end when attempting to remove the current tail node, as the tail cant be unlinked for thread safety. The new 'marked node' functionlity within the SubscriptionList allows cleaning up any nodes the prior 'lastSubscriptionNode' reference in the queue would have leaked by simply searching for the next non-deleted node after the marker, since this is guaranteed to be within the active list, after which point they are no longer leaked due to the above changes. Thanks, Robbie.
          Hide
          Robbie Gemmell added a comment -

          Just a note, QPID-3319 should be closed out at the same time as this is (without a fix-for; no point having 2 JIRAs for the same issue in the release notes).

          Show
          Robbie Gemmell added a comment - Just a note, QPID-3319 should be closed out at the same time as this is (without a fix-for; no point having 2 JIRAs for the same issue in the release notes).
          Hide
          Robbie Gemmell added a comment -

          Bumping this back to In Progress because I just thought of a corner case I didnt cover when cleaning up the marker node, will fix it up soon.

          Show
          Robbie Gemmell added a comment - Bumping this back to In Progress because I just thought of a corner case I didnt cover when cleaning up the marker node, will fix it up soon.
          Hide
          Robbie Gemmell added a comment -

          Added a test to pick up the problem, then fixed up the issue, which was: the marker cleanup could run off the end of the list if the previously-tail node was being removed, and so would end up pointing a dummy marker nodes 'next' reference at null instead of the new list tail, meaning it then wouldnt pick up future Subscriptions added to the list, which would cause the queue to cycle back to the list head preumaturely.

          Back over to you Keith.

          Show
          Robbie Gemmell added a comment - Added a test to pick up the problem, then fixed up the issue, which was: the marker cleanup could run off the end of the list if the previously-tail node was being removed, and so would end up pointing a dummy marker nodes 'next' reference at null instead of the new list tail, meaning it then wouldnt pick up future Subscriptions added to the list, which would cause the queue to cycle back to the list head preumaturely. Back over to you Keith.
          Hide
          Keith Wall added a comment -

          Reviewed, no comments.

          Show
          Keith Wall added a comment - Reviewed, no comments.

            People

            • Assignee:
              Keith Wall
              Reporter:
              Robbie Gemmell
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development