[MGNLBACKUP-28] "Unclosed session detected" warning messages Created: 29/Jan/10  Updated: 29/Mar/22  Resolved: 01/Jul/11

Status: Closed
Project: Backup
Component/s: None
Affects Version/s: 1.1.1
Fix Version/s: 1.1.3

Type: Improvement Priority: Minor
Reporter: Zdenek Skodik Assignee: Jan Haderka
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: File MGNLBACKUP-28.diff     File backup.log     File restore.log    
Issue Links:
relation
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Date of First Response:
Team: Nucleus

 Description   

The Jackrabbit's SessionImpl class logs a warnings during backup/restore because the session was not properly logged out.
This hasn't any impact when the finalize method executes the logout anyway but we would fix it to save users from feeling that something would be wrong.
See the attachments for details.



 Comments   
Comment by Sean McMains [ 02/Jun/11 ]

The problem seems to be that finalize() doesn't get called reliably, and that, per the language spec, subclasses of an object (Backup, in this case) don't call their superclass's finalize() method.

Since these warning messages were causing concern for one of our customers I'm working with, I put together a patch that appears to successfully get rid of these messages both during backup and restore by logging out all of the session objects that we open.

Comment by Tobias Mattsson [ 21/Jun/11 ]

It's not easy to follow the code path and verify that there's cleanup for everything that's initialized.

In the BackupManager there's explicit calls to Backup.init() of the backups in its config, that's okay as long as all Backups given to it haven't initialized a session in its constructor. Some do, though as far as I can see there are no code paths like that.

Many classes extending Backup have an init() method that appears to not be used, if it was used it could result in already created sessions being lost as the field is overwritten.

Closing sessions is done by accessing the protected field session in Backup, this works only for classes that extends Backup. So in order to use an object that extends from Backup and not loose sessions you have to extend from Backup yourself in the class that will use the Backup. UPDATE: there's also a getter for session that is also protected.

Would it not be easier to have two methods on Backup, that are part of their api/contract that do init() and dispose(). In order to use the Backup you have to call init() first and dispose() afterwards. This would break the API. These methods would be candidates for a try-finally block.

Comment by Tobias Mattsson [ 21/Jun/11 ]

In BackupManager.backup() inside the while loop, if the if-statement evaluates to true a session is lost since the instance being considered has been initialized but closing its session is skipped.

Also the session in BackupManager is created and properly disposed of but it is never used.

Comment by Tobias Mattsson [ 21/Jun/11 ]

Closing sessions should be handled in a dispose() method. The method should be public, see earlier comment. BackupManager should in its dispose() method call super the iterate and close all its resources.

Calls to dispose() should happen in a try-finally block.

Comment by Jan Haderka [ 22/Jun/11 ]

closed all additional sessions. Now calling finalize() (which was already there) consistently on each backup/restore run of each <? extends Backup> class.

Comment by Tobias Mattsson [ 30/Jun/11 ]

finalize() in BackupManager (or maybe in Backup) should also finalize Backup instances in BackupConfig.allResources.

public void finalize() {
	for (Iterator it = this.getConf().getAllResources().iterator(); it.hasNext(); ) {
		Backup b = (Backup) it.next();
		b.finalize();
	}
	super.finalize();
}
Comment by Jan Haderka [ 01/Jul/11 ]

Done. Change had to be in BackupManager since Backup is an abstract class and not all the other extending classes manipulate other backup instances. Also it should be responsibility of each class to finalize itself properly, but for the safety sake it's ok to do it again in the BM.

Generated at Sun Feb 11 23:24:52 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.