channels, commons pool, and memory leak : a feedback

[damien.grandemange]

Hi,

Lately, i have been trying to create a pool of channels (for a 'one shot' use of these channels) using Apache Commons Pool version 1.6.
Note that this thread is more a feedback actually. There's absolutely no issue with jPos channels and Apache Commons Pool. I rather want to focus on some inconvenience that occurred while trying to tight them.

In Apache Commons Pool, to create a pool of objects, one often uses the provided base implementation org.apache.commons.pool.impl.GenericObjectPool.
From this pool, a client code may borrow an instance when it needs it :
    channel = channelPool.borrowObject();
and must return it to the pool once it's done with it :
    channelPool.returnObject(channel);

Now, when creating the pool, one must provide a factory implementing org.apache.commons.pool.PoolableObjectFactory . This interface describes how to  to create, destroy, passivate instances of the pool.
Base implementation org.apache.commons.pool.BasePoolableObjectFactory provides a no op implementation of these methods.
So, to create and initialize channel instances, one can provide his own factory implementation and override the BasePoolableObjectFactory.makeObject() method.
Eventually, when a new channel instance will be needed in the pool, pool internals will delegate channel instanciation to this method. If everything gets fine, a new channel will be available in the pool.

Working with Apache Commons Pool, one may want to configure its pool to hold a number of idle instances in a [minIdle, maxIdle] interval.
An eviction thread runs every timeBetweenEvictionRunsMillis milliseconds and chases instances stayed idle for more than minEvictableIdleTimeMillis milliseconds in the pool.
This can be done by providing a org.apache.commons.pool.impl.GenericObjectPool.Config when creating the pool.
Eventually, pool instanciation code could look like this :

    poolConfig.maxActive = 25;
    poolConfig.maxIdle = 12;
    poolConfig.minIdle = 7;
    poolConfig.minEvictableIdleTimeMillis = 120000;
    poolConfig.numTestsPerEvictionRun = 25;
    poolConfig.timeBetweenEvictionRunsMillis = 60000;

    channelPool = new GenericObjectPool<ISOChannel>(poolFactory, poolConfig);

With such configuration, pool holds at least 7 idle instances in the pool. It also runs a=n eviction thread every minute to chase instances stayed idle in pool for more than  3 minutes.

I used this kind of configuration and it worked quite fine. Still ...

Looking at the JVM memory through the jconsole, memory graph showed that heap used memory was growing and growing, even though the app wasn't processing any dummy incoming requests. So what ?
Periodic manual GC never returned the memory back to it's lower level, and eventually, 12 hours later, boum ... the mercyless OutOfMemoryException came up.

So what happened ?
First, i must say that i wasn't aware at all that this memory leak was tighed to the <jPos channel, commonos pool> couple. So i many times ran the jmap java tool against my running app and compared successive class histograms.
Doing this, I noticed the [Lorg.jpos.iso.ISOFieldPackager instances number was keeping growing. How could that be ? After all, the app wasn't processing anything at all.
Wondering if my pool implementation was buggy, i put some extra logs in the BasePoolableObjectFactory.makeObject() method of my channel factory.
Looking at the instance creation frequency, I realized that, when the commons pool eviction thread runs, even if the pool holds a sufficient number of idle instances, any too older instances (age > minEvictableIdleTimeMillis) are re-created (that means destroyed and re-make).

One of the possible explanation of the memory leak was that the evicted channel instances were retained by some other object in the memory. My implementation wasn't doing retention, so i check the jPos QFactory, but there was nothing suspect there either.
Taking a look at Q2 logs, the sysmon dump was showing that the older channels were still listed : could it be that the channels were retained by the NameRegistrar ?
Looking deeper into jPos BaseChannel implementation, i saw that the setName(String name) setter method was also caring of channel instance registration in the NameRegistrar. 
Well, don't know if there is a better way, but i eventually fixed this by overriding the pool's object factory BasePoolableObjectFactory.destroyObject(ISOChannel channel) method like this :
    public void destroyObject(ISOChannel channel) throws Exception {
        NameRegistrar.unregister("channel." + channel.getName());
    }

No more leaks now.  Phew ...

[chhil]

Looking at the channelAdaptor class the startService registers the channel and destroyService unregisters it. It seems like the basechannels setName is rather redundant (I could be wrong). Maybe if pools destroy calls the appropriate bean to stop/destroy may be a cleaner way to do it?

-chhil

--
--
jPOS is licensed under AGPL - free for community usage for your open-source project. Licenses are also available for commercial usage.
Please support jPOS, contact: sa...@jpos.org
 
You received this message because you are subscribed to the "jPOS Users" group.
Please see http://jpos.org/wiki/JPOS_Mailing_List_Readme_first
To post to this group, send email to jpos-...@googlegroups.com
To unsubscribe, send email to jpos-users+...@googlegroups.com
For more options, visit this group at http://groups.google.com/group/jpos-users
 
---
You received this message because you are subscribed to the Google Groups "jPOS Users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jpos-users+...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[Alejandro Revilla]

Hi Damien,

I think we should remove this call to setName in OneShotChannelAdaptor: 

   https://github.com/jpos/jPOS/blob/master/jpos/src/main/java/org/jpos/q2/iso/OneShotChannelAdaptor.java?#L230-231

I can't think of a use case for that giving that all communication with the service happens through a space queue.

Would that solve your issue?

--
@apr

[chhil]

Alejandro,
The behavior you point out is in the ChannelAdaptor too.
If the qbeans object lifecycle methods start/stop/etc are used in the custom pool, that should solve the problem, I would think.

-chhil

[Alejandro Revilla]

 

The behavior you point out is in the ChannelAdaptor too.

For some reason I thought Damien was playing wiht the one shot channel adaptor, not the regular one.

 

If the qbeans object lifecycle methods start/stop/etc are used in the custom pool, that should solve the problem, I would think.

Problem is when you stop one channel you don't want to stop the whole adaptor.

[chhil]

There is a one to one relationship between them hence I was of the opinion to stop the adaptor bean. And the adaptor does take care of removing the channel.

[damien.grandemange]

Actually, i am working on the one shot channel adaptor i initiated last year. (see this discussion https://groups.google.com/d/topic/jpos-users/wzhQUqF-qPo/discussion ). But my implementation was somehow buggy in some case.
(Initial need was to be able to manage failover/round-robin on one shot channel adapators.)
Now, i am trying to rewrite it more seriously because it has a major bug in it.

To do so, i am using Apache Commons Pool and Executor framework. I am not working any more with the inner OneShotChannelAdaptor.Worker thread, though i re-use all the Worker.initChannel() mechanism.
And doing so, i indeed use the famous setName() that you mentionned, Alejandro.

So, Alejandro, about your suggestion : there's no need to update jPos current OneShotChannelAdaptor implementation because i am not using this one for my purpose (though i pick ideas from it), i am working on a new one. But, you are correct: the line you mentionned is the one that cause the channel to be registered in NameRegistrar.

[Alejandro Revilla]

Excellent. Then perhaps you can remove that line in your implementation.

I know it means extra configuration, but perhaps it's simpler for you use a set of regular OneShotChannelAdaptors with the MUXPool to implement your round-robin strategy. MUXPool could use some love to implement more flexible strategies ...

--
@apr

[damien.grandemange]

Well, we already had a discussion on this subject and it seems to me that MUXPool relies on the ready state of its underlying muxes. Due to the unpersistent nature of one shot channels, this cannot be achived so simply, i think. That's why i was working on a new implementation. Or may be i've mised something.

[Alejandro Revilla]

Correct, but perhaps the OneShotChannelAdaptor could be smarter at the time of setting the ready indicator in the Space (thinking out loud...) even doing some probes.

--
@apr

On Fri, Jun 28, 2013 at 12:49 PM, damien.grandemange <damien.gr...@gmail.com> wrote:

Well, we already had a discussion on this subject and it seems to me that MUXPool relies on the ready state of its underlying muxes. Due to the unpersistent nature of one shot channels, this cannot be achived so simply, i think. That's why i was working on a new implementation. Or may be i've mised something.

--

[damien.grandemange]

Well i think cchil's one shot Watch Dog solution is doing it this way.
But i am trying something else. I don't want to be "agressive" with the acquirer (that is i don't want to establish dummy connections just for readyness purpose). And actually, i would like to know if my acquirer is available by the very time i am trying to connect to it.
This is another approach i am trying to take.

[chhil]

If I recollect the previous conversation, the idea was to get away from the ready channel, as it really does not make sense for a one shot, you want to have a payload ready, connect, deliver, get a response and disconnect. The ready thing is an unnecessary synchronizing overhead in the one shot case. Ready makes sense for persistent connections.

-chhil

[Alejandro Revilla]

I remember. 

The MUX, and specially the MUXPool can benefit from some additional info about its channels beyond just the in/out queue names. Right now we just have a true/false indicator with the 'ready' strategy. We could provide a more interesting object that could tell us more about the channel, including some stats from previous transactions or at least a float between 0 and 1 telling us some kind of quality value. 

We enter deep waters with that, because MUXPool decisions about which channel to use may influence those stats and we may end up favoring a single channel, but anyway, good stuff to keep in mind.

--
@apr

[Andrés Alcarraz]

El 28/06/13 14:02, Alejandro Revilla escribió:

I remember. 

The MUX, and specially the MUXPool can benefit from some additional info about its channels beyond just the in/out queue names. Right now we just have a true/false indicator with the 'ready' strategy. We could provide a more interesting object that could tell us more about the channel, including some stats from previous transactions or at least a float between 0 and 1 telling us some kind of quality value.

I guess that value could be received/sent over a window, and the selection could be something like selecting the channel with probability proportional to its quality, i wonder if this wouldn't be overkilling but sounds nice to me:).

To not make use of the overkilling of randomness of the decision in each transaction we can make a bucket of let's say 100 or 1000 or some number proportional to the channels, and asign buckets proportionally to the quality, and update that assignment on certain conditions (e.g. when a quality change too much). Then we make round robin on the buckets.

The random calls should be made only when assigning buckets.

Just thinking in loud too but sounded interested enough to think a little.

I Hope this is more nuts than noise :).

And

-- 
And

[Alejandro Revilla]

I Hope this is more nuts than noise :).

Good food for thought!

[damien.grandemange]

I eventually commit my new implementations (see initial demo project here https://github.com/dgrandemange/OneShotChannelPool-demo). Happy with it for now.

(Talking about indicators and sliding windows, may be you could take a look at the Esper framework.)

[chhil]

Cool. Will give it shot :)

-chhil

On Wed, Jul 3, 2013 at 3:40 PM, damien.grandemange <damien.gr...@gmail.com> wrote:

I eventually commit my new implementations (see initial demo project here https://github.com/dgrandemange/OneShotChannelPool-demo). Happy with it for now.

(Talking about indicators and sliding windows, may be you could take a look at the Esper framework.)

[Alejandro Revilla]

Great! Will take a look a-s-a-p.

--
@apr