On Tue, 2017-11-07 at 09:51 +0000, Robbie Gemmell wrote: > On 6 November 2017 at 14:42, Lorenz Quack <[email protected]> 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 <[email protected]> > > > > wrote: > > > > > > > > > > > > > > > > > > > > On 1 November 2017 at 10:16, Lorenz Quack <[email protected]> > > > > > 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: [email protected] > > For additional commands, e-mail: [email protected] > > > 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 --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
