qpid-users mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Robbie Gemmell <robbie.gemm...@gmail.com>
Subject Re: Accumulation of Links from global shared durable subscriptions (JMS 2.0)
Date Tue, 07 Nov 2017 11:10:38 GMT
On 7 November 2017 at 10:26, Lorenz Quack <quack.lorenz@gmail.com> wrote:
> On Tue, 2017-11-07 at 09:51 +0000, Robbie Gemmell wrote:
>> On 6 November 2017 at 14:42, Lorenz Quack <quack.lorenz@gmail.com> wrote:
>> >
>> > On Mon, 2017-11-06 at 11:07 +0000, Lorenz Quack wrote:
>> > >
>> > > On Mon, 2017-11-06 at 10:14 +0000, Robbie Gemmell wrote:
>> > > >
>> > > >
>> > > > On 3 November 2017 at 16:58, Rob Godfrey <rob.j.godfrey@gmail.com>
wrote:
>> > > > >
>> > > > >
>> > > > >
>> > > > > On 1 November 2017 at 10:16, Lorenz Quack <quack.lorenz@gmail.com>
wrote:
>> > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > > Hi,
>> > > > > >
>> > > > > > We noticed a potential problem with the way JMS 2.0 global
shared
>> > > > > > durable subscriptions are implemented in the JMS client.
 The
>> > > > > > implementation was designed, discussed, and implemented
in QPIDJMS-220
>> > > > > > [1].
>> > > > > >
>> > > > > > When a consumer of a subscription closes the underlying
AMQP link is
>> > > > > > not closed but merely detached since the closing of a subscription
>> > > > > > link is used as the signal to unsubscribe the subscription.
 These
>> > > > > > subscription links remain on the broker until the subscription
is
>> > > > > > unsubscribed (detach with close=true) at which point the
broker
>> > > > > > discards all links associated with the subscription.
>> > > > > >
>> > > > > > For non-global subscriptions this does not seem to be a
problem since
>> > > > > > the clientId is used as the container-id and links are reused
if
>> > > > > > possible.
>> > > > > >
>> > > > > > For global subscription on the other hand the client uses
a random
>> > > > > > UUID associated with the connection as its own container-id.
 This
>> > > > > > means that on every new connection a new link is being estblished
>> > > > > > rather than recovering an existing one.  Over time these
subscription
>> > > > > > links would accumulate on the broker until the subscription
is
>> > > > > > unsubscribed.
>> > > > > >
>> > > > > > Since shared durable subscriptions are expected to be long
lived this
>> > > > > > raises some concerns with regards to resource exhaustion
on the
>> > > > > > broker.  An application that would spin up a connection
do some work
>> > > > > > on a shared durable global subscription and then disconnect
again
>> > > > > > would be "leaking" links.
>> > > > > >
>> > > > > > Due to this concern we plan on disabling shared durable
global
>> > > > > > subscriptions for the initial v7 release of Qpid Broker-J.
 There will
>> > > > > > be a context variable to enable the feature.
>> > > > > >
>> > > > > >
>> > > > > It seems to me a real shame that we are disabling this feature
of JMS 2.0
>> > > > > because of the fact that from an AMQP point of view the link
*may* be
>> > > > > resumed, but in practice, with JMS, will never be.  Would it
not be better
>> > > > > to have a context variable which affects the behaviour of such
links to
>> > > > > shared durable subscriptions such that if the context variable
returns
>> > > > > "true" the links are removed from the link registry at the time
of
>> > > > > connection closure, and if the context value is false, then the
current
>> > > > > behaviour is maintained.  We could then later add functionality
to the
>> > > > > client to specify (via some flag) the behaviour it desires (thus
ultimately
>> > > > > removing the need for the context variable)?
>> > > > >
>> > > > > -- Rob
>> > > > >
>> > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > > Thoughts, comments, discuss!
>> > > > > >
>> > > > > > Kind regards,
>> > > > > > Lorenz
>> > > > > >
>> > > > > >
>> > > > > > [1] https://issues.apache.org/jira/browse/QPIDJMS-220
>> > > > > >
>> > > > I agree that disabling-by-default one of the more useful feature
>> > > > additions seems a real shame. The timing was dissapointing also.
>> > > >
>> > > > The alternative idea for the context variable sounds good to me, as
>> > > > does having the client flag its behaviour in some way.
>> > > >
>> > > > Robbie
>> > > Thanks for all the feedback.
>> > > We are looking into options to make this work without leaking Links.
>> > > Depending on how impactful/intrusive the change would have to be we
>> > > might consider including this in v7.0.0 since Rob and Robbie seem to
>> > > feel quite strongly about this.
>> > >
>> > > Regarding the timing, we felt that shipping the broker with a known
>> > > memory leak was undesirable and since this is not a regression but
>> > > a new feature, disabling the new feature by default until a proper
>> > > solution (with a potential change to QPIDJMS-220 and the client)
>> > > could be devised seemed like a reasonable way forward.  I am sorry
>> > > that you feel differently.  I hope we can come up with a solution
>> > > that is acceptable to everyone.
>> > >
>> > >
>> > > Kind regards,
>> > > Lorenz
>> > >
>> > Hi,
>> >
>> > I discussed the above with Keith and Alex and we decided that we
>> > are willing to change this in v7.0.0 and spin another RC.
>> >
>> > I attached a patch to QPID-7998 [1] with our proposed change.
>> > Below is a description of the patch.  Please indicate whether
>> > you find these changes acceptable so we can respin the RC soon.
>> >
>> >
>> > Longer version:
>> > ===============
>> > When attaching a new link will always be created for global shared
>> > subscriptions.  This is different from before where if it is the same
>> > connection the link would be recovered.  But I do not think that this
>> > is observable by a client (other than through dumpLinkRegistry).
>> >
>> > When the subscription is ended (unsubscribe) then we need to perform a
>> > null-source lookup in the registry.  In our previous work we already
>> > changed this from vanilla AMQP 1.0 to allow the lookup to include a
>> > search for different remote container-ids.  I did not change this.
>> > Instead when detaching we ensure that there remains at least one link
>> > in the registry representing the subscription in order to facilitate
>> > the unsubscribe.
>> >
>> > The change is relatively small and only touches SendingLinkEndpoint if
>> > you disregard the changes necessary to undo the former code to
>> > disallow global shared subscriptions.
>> >
>> > The previous context variable
>> >   qpid.feature.disabled:globalSharedDurableSubscription
>> > has been removed in favour of a new one
>> >   qpid.jms.discardGlobalSharedSubscriptionLinksOnDetach
>> > Obviously, the semantics have changed.
>> >
>> > TL;DR:
>> > ======
>> > The attached patch should allow global shared subscriptions,
>> > not leak links in the registry, and give us a certain amount of
>> > confidence that it does not affect other areas of the code.
>> >
>> > Kind regards,
>> > Lorenz
>> >
>> > [1] https://issues.apache.org/jira/browse/QPID-7998
>> >
>> > ---------------------------------------------------------------------
>> > To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
>> > For additional commands, e-mail: users-help@qpid.apache.org
>> >
>> Hi Lorenz,
>>
>> This sounds/looks good to me overall, though I wonder around whether
>> the 'more than 1 left' checking + subsequent calls are racey (e.g
>> across different connections with subscribers detaching at the same
>> time) and if so about the consequences.
>>
>> Robbie
>
> Hi Robbie,
>
> Well spotted. It is known to be racey.
> In the worst case the last two links would unsubscribe concurrently and
> both be removed from the link registry.  Upon unsubscribe the link would
> not be found in the registry and the attach would be rejected with
> "not-found". The subscription queue would remain on the broker.
> There are other races connected to shared subscriptions / link registry.
> That does not make it better but we really want to get the release out
> so we created QPID-7996 to address these issues in 7.0.1.
>
> We are going to back port the change an spin a RC2 today.
>
> Kind regards,
> Lorenz
>

Ok. Not ideal, but nothing like as bad as the subscription queue being
removed prematurely, which was my main concern when I saw it.

Robbie

---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
For additional commands, e-mail: users-help@qpid.apache.org


Mime
View raw message