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
|