commons-user mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Oliver Zeigermann" <oliver.zeigerm...@gmail.com>
Subject Re: [transaction] Memory leaks in GenericLockManager, FileResourceManager
Date Tue, 23 May 2006 11:14:38 GMT
Hi Antranig!


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?

>
> 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.

> 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.

Cheers

Oliver

---------------------------------------------------------------------
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