activemq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Manuel Teira <mte...@tid.es>
Subject Re: Advisory topic leakages (second theory)
Date Tue, 10 Jul 2007 07:02:42 GMT
Rob Davies escribió:
> hi Manuel,
>
> on the face of it - this does look like an issue was introduced by the 
> change in trunk  you mentioned. I think the code was changed to help 
> some concurrency issues. However, the broker has changed considerably 
> since then, so I'm not sure it still applies. The best thing to do 
> would be to apply your patch + rollback the change in AbstractBroker 
> you highlighted, and see if anything breaks ;)
Well, perhaps I didn't explain it correctly. The changes I was talking 
about (destinations created inside the TopicRegion and not being created 
from top) are only applied  in the trunk (509706), not in  the 4.1 branch.
The fact is that once I found the problem, and knowing that the trunk 
was not suffering the leakage problem I'm behind, I wanted to know the 
reason, and found these differences.

What I would like to know is how is it now (now=trunk) supposed to work. 
Changes in 509706 are, if I understand correctly, avoiding the 
auto-created topics  to be exposed in the RegionBroker. So, when the 
AdvisoryBroker tries to delete them, propagating the removeDestination 
to the chained brokers, it won't find them. True?

Regards.

>
> cheers,
>
> Rob
>
> On Jul 9, 2007, at 1:57 PM, Manuel Teira wrote:
>
>> And even worse, creating the destination in this way (inside the 
>> TopicRegion) is not going to make the RegionBroker aware of it. Since 
>> the AdvisoryBroker is relaying in the broker chain to try to delete 
>> the consumer and producer advisory topics when a destination is 
>> deleted, how and when are they supposed to be deleted in the trunk code?
>>
>> Regards.
>>
>>
>>
>> Manuel Teira escribió:
>>> Hi again.
>>>
>>> I'm afraid that the lookup code in AbstractRegion has being changed 
>>> in the trunk (I was looking at 4.1 branch). Basically, instead of 
>>> calling context.getBroker().addDestination(context,destination) to 
>>> create the new destination, addDestination(context, destination) is 
>>> used. This way, the advisory topic won't be created from top, but 
>>> what happens with the advise in the code?
>>>
>>>                if(autoCreateDestinations){
>>>                    // Try to auto create the destination... 
>>> re-invoke broker from the
>>>                    // top so that the proper security checks are 
>>> performed.
>>>                    try {
>>>                        dest = addDestination(context, destination);
>>>                        
>>> //context.getBroker().addDestination(context,destination);
>>>                    }
>>> I suppose that the assumption is no longer true.
>>>
>>> Also, the way to change this code (only commenting out the old one) 
>>> makes me think about a not too mature change?
>>>
>>> If you can verify that this is the right way to proceed,I will like 
>>> to prepare a patch against 4.1 branch.
>>>
>>>
>>> Best regards.
>>>
>>>
>>> Manuel Teira escribió:
>>>> Hello again. Digging into the problem I've found another thing 
>>>> related with an asymmetry in the way an advisory topic  is created 
>>>> and destroyed.
>>>>
>>>> I'm analizying the way the Consumer and Producer advisory topics 
>>>> for temporary queues are created and destroyed:
>>>>
>>>> An advisory topic is actually created when the AdvisoryBroker 
>>>> fireAdvisory method is eventually sending the message. This is 
>>>> happening in AbstractRegion lookup method, as the advisory topic 
>>>> doesn't exist yet:
>>>>
>>>>    protected Destination lookup(ConnectionContext context, 
>>>> ActiveMQDestination destination) throws Exception {
>>>>        synchronized(destinationsMutex){
>>>>            Destination dest=(Destination) 
>>>> destinations.get(destination);
>>>>            if(dest==null){
>>>>                if(autoCreateDestinations){
>>>>                    // Try to auto create the destination... 
>>>> re-invoke broker from the
>>>>                    // top so that the proper security checks are 
>>>> performed.
>>>>                    
>>>> context.getBroker().addDestination(context,destination);
>>>>                    // We should now have the dest created.
>>>>                    dest=(Destination) destinations.get(destination);
>>>>                }
>>>>                if(dest==null){
>>>>                    throw new JMSException("The destination 
>>>> "+destination+" does not exist.");
>>>>                }
>>>>            }
>>>>            return dest;
>>>>        }
>>>>    }
>>>>
>>>> Hence, the whole Broker chain is called to create a destination 
>>>> (context.getBroker().addDestination), this, in a common 
>>>> environment, involves calling:
>>>>
>>>> MutableBrokerFilter.addDestination - Just pass the request to the 
>>>> next chained BrokerFilter
>>>> [
>>>>   Here the configured plugins
>>>> }
>>>> CompositeDestinationBroker. No implementation, so it passes the 
>>>> request to the next chained object.
>>>> AdvisoryBroker. Fires an advisory to the destination advisory 
>>>> topic, and adds the destination to its own destinations map.
>>>> TransactionBroker.  No implementation, passes the request to the 
>>>> next chained object.
>>>> RegionBroker. Delegates in the TopicRegion.addDestination to create 
>>>> the given advisory topic.
>>>>
>>>> On the other way, this advisory topic is destroyed when the advised 
>>>> destination is removed, in AdvisoryTopic. removeDestinationInfo. 
>>>> But here, the way to do it is:
>>>>
>>>>    public void removeDestinationInfo(ConnectionContext context, 
>>>> DestinationInfo destInfo) throws Exception{
>>>>        next.removeDestinationInfo(context, destInfo);
>>>>        DestinationInfo info = (DestinationInfo) 
>>>> destinations.remove(destInfo.getDestination());
>>>>
>>>>        if( info !=null ) {
>>>>            info.setDestination(destInfo.getDestination());
>>>>            
>>>> info.setOperationType(DestinationInfo.REMOVE_OPERATION_TYPE);
>>>>            ActiveMQTopic topic = 
>>>> AdvisorySupport.getDestinationAdvisoryTopic(destInfo.getDestination()); 
>>>>
>>>>            fireAdvisory(context, topic, info);
>>>>            try {
>>>>                next.removeDestination(context, 
>>>> AdvisorySupport.getConsumerAdvisoryTopic(info.getDestination()), -1);
>>>>            } catch (Exception expectedIfDestinationDidNotExistYet) {
>>>>            }
>>>>            try {
>>>>                next.removeDestination(context, 
>>>> AdvisorySupport.getProducerAdvisoryTopic(info.getDestination()), -1);
>>>>            } catch (Exception expectedIfDestinationDidNotExistYet) {
>>>>            }
>>>>        }
>>>>
>>>>    }
>>>>
>>>>
>>>> So, only the next chained broker components to AdvisoryBroker are 
>>>> called to remove the consumer and producer advisory topics. This 
>>>> way to proceed suggests me two problems:
>>>>
>>>> 1.-The advisory broker itself is not aware of the deletion of those 
>>>> topics (remember that it had registered them when the whole broker 
>>>> chain was called to create the topic). I think that this is the 
>>>> leakage I'm suffering.
>>>> 2.-Any plugin (or component in the chain preceding the 
>>>> AdvisoryBroker) that could be creating and retaining objects 
>>>> related with these advisory topics won't never be able to release 
>>>> them.
>>>>
>>>> Perhaps this way to proceed could be related with the fix of AMQ-677.
>>>>
>>>> Did I miss anything?
>>>>
>>>> Regards.
>>>>
>>>>
>>>>
>>>
>>>
>>
>
>
>
> Rob Davies
> 'Go further faster with Apache Camel!'
> http://rajdavies.blogspot.com/
>
>
>
>


Mime
View raw message