commons-user mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Antranig Basman <antra...@caret.cam.ac.uk>
Subject Re: [transaction] Memory leaks in GenericLockManager, FileResourceManager
Date Tue, 23 May 2006 16:48:53 GMT
Oliver Zeigermann wrote:
> 
> Hi Antranig!

Hi!

> 2006/5/23, Antranig Basman <antranig@caret.cam.ac.uk>:
> > The most straightforward of them is in GenericLockManager, where
> > the lock owner is not being released properly - I have
> > the following:
> >
> > Index:
> > E:/workspace/commons-transaction-test/src/java/org/apache/commons/transaction/locking/GenericLockManager.java
> > ===================================================================
> > ---
> > E:/workspace/commons-transaction-test/src/java/org/apache/commons/transaction/locking/GenericLockManager.java
> > (revision 408762)
> > +++
> > E:/workspace/commons-transaction-test/src/java/org/apache/commons/transaction/locking/GenericLockManager.java
> > (working copy)
> > @@ -337,6 +337,8 @@
> >                  lock.release(ownerId);
> >                  locks.remove(lock);
> >              }
> > +            // AMB fix leak
> > +            globalOwners.remove(ownerId);
> >          }
> >      }
> 
> Hmmm. Do you argue that the owner can be removed from the list of
> owners as it no longer holds any locks at this particular point of
> time? That's what I presume.
> 
> I wonder, is it possible that another thread acquires a new lock for
> that specific owner in the meantime? So that this removal would
> discard that. Which of course would be an error.
> 
> Is that possible?
> 
> Anyway, there really should be a way to get rid of an owner if it
> really is no longer needed. Maybe a new method for that?

OK, perhaps not even this leak is straighforward. 

I'm worried about adding more APIs in this area since the
ownership/scoping rules are already difficult enough for me to
understand. If I understand your point correctly, this means that
what we require inside GLM is an "automated" scheme such that,
at *any point* where the last lock is removed for an owner,
bringing the set size down to zero, the entire globalOwners
entry is collected, in some kind of thread-safe way.
I.e. this condition, presumably, could even be triggered from
"removeOwner".
Perhaps a private/protected member entitled 
"checkLocksEmpty(Object ownerId)" which will synchronize on
globalOwners, and then remove the Locks map for it if it is
empty?

This sounds expensive, since globalOwners would be a top-level
lock, but I can't think of a more clever scheme at the moment.


In general I'm afraid of the very large amount of locking I see
going on throughout transaction, but understandably given it is
managing set of locks, quite a lot of this is probably 
unavoidable(!).

However I think performance could be considerably improved if
there were to be a movement over to one of Doug Lea's kinds of
Concurrent Hashmaps, probably from his older "oswego" or
"backport" packages so we could maintain JDK 1.4 compliance.

What sort of load/throughput testing has been done on 
commons-transaction?

> > There seem to be two leaks in FileResourceManager,
> > but I gave up trying to patch them precisely on seeing that they
> > both centred around the globalOpenResources table, which as far
> > as I can see is not used anywhere in the codebase, and only
> > seems to serve the function of hanging onto FileInputStream objects
> > long after they should have been collected :P - after a few attempts I
> > just commented out all references to this member entirely.
> 
> globalOpenResources keeps track of all open resources. That is
> important as they will have to be closed when the manager is shut
> down.
> 
> > Can we just get away with synchronizing on "openResourcs" inside
> > context.closerResources() and context.registerResource?
> 
> No need to synchronize on openResourcs (oops, there is a typo!). What
> for? Those methodes are synchronized (on the context) anyway.

I was just noticing that these context-scope methods were actually
using the globalOpenResources as their sync target. In the model 
where we were trying to get rid of globalOpenResources entirely I
was wondering whether it would make sense to have picked a 
more local target anyway. But as you say below, gOR is actually
required for coherence...

> 
> > If anyone is interested I could renew my efforts to find more
> > fine-grained patches - for example in the current version rev
> > 375638 there should definitely at line 1628 be a call to
> >
> >  globalOpenResources.remove(is);
> >
> > following the call to
> > globalTransactions.remove(txId);
> >
> > I can't recall where the 3rd leak site was, but it was certainly
> > resolved by commenting out globalOpenResources.
> 
> That seems reasonable to me. However, the whole topic is so
> complicated I wouldn't want to change that before more intense
> thinking. I would be pleased if you could have a closed look to those
> leaks and test the patches properly.

OK, I see that the globalOpenResources table is indeed required for
coherence. I will try to compose a patch with it incorporated that
does not leak - unfortunately I will be away for the next two
weeks and so will not be able to look at it very quickly.

Cheers,
Antranig.

---------------------------------------------------------------------
To unsubscribe, e-mail: commons-user-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-user-help@jakarta.apache.org


Mime
View raw message