On 7 November 2017 at 10:26, Lorenz Quack <quack.lor...@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.lor...@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.godf...@gmail.com> >> > > > wrote: >> > > > > >> > > > > >> > > > > >> > > > > On 1 November 2017 at 10:16, Lorenz Quack <quack.lor...@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-unsubscr...@qpid.apache.org >> > For additional commands, e-mail: users-h...@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: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
