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 --------------------------------------------------------------------- To unsubscribe, e-mail: users-unsubscr...@qpid.apache.org For additional commands, e-mail: users-h...@qpid.apache.org
