Memory leak is possible if changes for audited entities are outside of transaction

Description

Envers always collects changes for audited objects in global (global per sessionFactory instance) application map (auditConfiguration.auditSyncManager.auditSyncs<Transaction, AuditSync>) even if transaction is inactive.
It leads to memory leak in application if there were changes (insert/update/delete) for versioned objects outside of transaction (transaction.isActive() == false) because in this case transaction is not committed where collected changes for current transaction are removed from global map.

Possible solution is patch Envers to check if transaction is active (AuditEventListener in onPostInsert/onPostUpdate/onPostDeleteonCollectionAction methods) before starting collect changes for audited object.
Skip collecting (to prevent memory leak) if transaction is inactive and log WARN message to indicate problem ("Couldn't create revision for entity ${entityName} because transaction is not active.")

Environment

hibernate-core-3.3.1.GA.jar, jboss-envers-1.2.1-hibernate-3.3.jar

Activity

Show:
Eugene Goroschenya
September 30, 2010, 7:06 PM

The previous

is incorrect because of WARN log message
log.warn("Couldn't create revision for entity " + entityName + " because transaction is not active.");
even in case of non-audited entry if transaction is inactive.
See

Erik-Berndt Scheper
October 1, 2010, 9:02 AM

Hi,

Unfortunately this patch is against an old version of Envers. Could you create a patch against the current Hibernate trunk (3.6.x), preferably together with a testcase?

Eugene Goroschenya
October 1, 2010, 10:38 AM

Yes, I will create patch for the current Hibernate trunk (3.6.x).

Regarding testcase not sure as don't know good way to check private field auditSyncManager.auditSyncs<Transaction, AuditSync>) contains not necessary data which never cleared.
The only idea maybe use EasyMock to verify that if auditSyncManager.get(eventSource) was called then auditSyncManager.remove(transaction) must be called too if transaction active and that auditSyncManager.get(eventSource) never is called if transaction is inactive.
The other problem with testcase is that EntityManager.flush() is only possible within active transaction but it is not restriction for Hibernate session, unfortunately testcases are used EntityManager (see AbstractEntityTest), so I will have to create testcase which uses Hibernate session directly (without EntityManager)

Lukasz Antoniak
June 17, 2012, 12:09 PM

IMO we could throw a runtime exception instead of just logging a warning message. In case of applying modifications outside of an active transaction, historical data would not be flushed to audit tables (AuditProcess#doBeforeTransactionCompletion(SessionImplementor) not executed), which is more serious issue. Adam, what do you think?

AdamA
June 17, 2012, 1:16 PM

+1 for throwing an exception. Such code indicates incorrect Hibernate/Envers usage.

Also +1 for extracting the actual check to a method so that the code isn't repeated

Fixed

Assignee

Lukasz Antoniak

Reporter

Eugene Goroschenya

Fix versions

Labels

None

backPortable

None

Suitable for new contributors

None

Requires Release Note

None

Pull Request

None

backportDecision

None

Components

Affects versions

Priority

Major