[ 
https://issues.apache.org/jira/browse/CLOUDSTACK-9564?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15625056#comment-15625056
 ] 

ASF GitHub Bot commented on CLOUDSTACK-9564:
--------------------------------------------

Github user abhinandanprateek commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/1729#discussion_r85904768
  
    --- Diff: 
vmware-base/src/com/cloud/hypervisor/vmware/util/VmwareContextPool.java ---
    @@ -45,76 +43,74 @@ public VmwareContextPool() {
             this(DEFAULT_IDLE_QUEUE_LENGTH, DEFAULT_CHECK_INTERVAL);
         }
     
    -    public VmwareContextPool(int maxIdleQueueLength) {
    -        this(maxIdleQueueLength, DEFAULT_CHECK_INTERVAL);
    -    }
    -
         public VmwareContextPool(int maxIdleQueueLength, long 
idleCheckIntervalMs) {
    -        _pool = new HashMap<String, List<VmwareContext>>();
    +        _pool = new HashMap<String, Queue<VmwareContext>>();
     
             _maxIdleQueueLength = maxIdleQueueLength;
             _idleCheckIntervalMs = idleCheckIntervalMs;
     
             _timer.scheduleAtFixedRate(getTimerTask(), _idleCheckIntervalMs, 
_idleCheckIntervalMs);
         }
     
    -    public void registerOutstandingContext(VmwareContext context) {
    -        assert (context != null);
    -        synchronized (this) {
    -            _outstandingRegistry.add(context);
    -        }
    -    }
    -
    -    public void unregisterOutstandingContext(VmwareContext context) {
    -        assert (context != null);
    -        synchronized (this) {
    -            _outstandingRegistry.remove(context);
    +    public VmwareContext getContext(final String vCenterAddress, final 
String vCenterUserName) {
    +        final String poolKey = composePoolKey(vCenterAddress, 
vCenterUserName);
    +        if (Strings.isNullOrEmpty(poolKey)) {
    +            return null;
             }
    -    }
    -
    -    public VmwareContext getContext(String vCenterAddress, String 
vCenterUserName) {
    -        String poolKey = composePoolKey(vCenterAddress, vCenterUserName);
             synchronized (this) {
    -            List<VmwareContext> l = _pool.get(poolKey);
    -            if (l == null)
    -                return null;
    -
    -            if (l.size() > 0) {
    -                VmwareContext context = l.remove(0);
    -                context.setPoolInfo(this, poolKey);
    -
    -                if (s_logger.isTraceEnabled())
    -                    s_logger.trace("Return a VmwareContext from the idle 
pool: " + poolKey + ". current pool size: " + l.size() + ", outstanding count: 
" +
    -                        VmwareContext.getOutstandingContextCount());
    +            final Queue<VmwareContext> ctxList = _pool.get(poolKey);
    +            if (ctxList != null && ctxList.size() > 0) {
    +                final VmwareContext context = ctxList.remove();
    +                if (context != null) {
    +                    context.setPoolInfo(this, poolKey);
    +                }
    +                if (s_logger.isTraceEnabled()) {
    +                    s_logger.trace("Return a VmwareContext from the idle 
pool: " + poolKey + ". current pool size: " + ctxList.size() + ", outstanding 
count: " +
    +                            VmwareContext.getOutstandingContextCount());
    +                }
                     return context;
                 }
    -
    -            // TODO, we need to control the maximum number of outstanding 
VmwareContext object in the future
                 return null;
             }
         }
     
    -    public void returnContext(VmwareContext context) {
    +    public void registerContext(final VmwareContext context) {
             assert (context.getPool() == this);
             assert (context.getPoolKey() != null);
             synchronized (this) {
    -            List<VmwareContext> l = _pool.get(context.getPoolKey());
    -            if (l == null) {
    -                l = new ArrayList<VmwareContext>();
    -                _pool.put(context.getPoolKey(), l);
    +            Queue<VmwareContext> ctxList = _pool.get(context.getPoolKey());
    +            if (ctxList == null) {
    +                ctxList = EvictingQueue.create(_maxIdleQueueLength);
    --- End diff --
    
    Since all access is in synchronized blocks this is not required. But a 
synchronised evictingqueue could have let you avoid some of synchronised blocks.
    
    Again, this is not required as it is.


> Fix memory leak in VmwareContextPool
> ------------------------------------
>
>                 Key: CLOUDSTACK-9564
>                 URL: https://issues.apache.org/jira/browse/CLOUDSTACK-9564
>             Project: CloudStack
>          Issue Type: Bug
>      Security Level: Public(Anyone can view this level - this is the 
> default.) 
>            Reporter: Rohit Yadav
>            Assignee: Rohit Yadav
>
> In a recent management server crash, it was found that the largest 
> contributor to memory leak was in VmwareContextPool where a registry is held 
> (arraylist) that grows indefinitely. The list itself is not used anywhere or 
> consumed. There exists a hashmap (pool) that returns a list of contexts for 
> existing poolkey (address/username) that is used instead. The fix would be to 
> get rid of the registry and limit the hashmap context list length for any 
> poolkey.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to