[jbossts-issues] [JBoss JIRA] Commented: (JBTM-503) Shutdown the ExpiredEntryMonitor when stopping the RecoveryManager

[Andrew Dinn (JIRA)]

    [ https://jira.jboss.org/jira/browse/JBTM-503?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12456453#action_12456453 ] 

Andrew Dinn commented on JBTM-503:
----------------------------------

Hi Mauro,

RecoveryManager.startRecoveryManagerThread() is called from TransactionManager.start(). There are actually two versions of this class located in the jbossatx.jta and jbossatx.jts packages. They provide the interface to the Application Server.

I had not noticed that the ExpiredEntryMonitor startup call is made in the constructor. This is probably not the correct place for it to happen but I htink there is a reason for it.

I'll note that the periodic recovery instance also gets created in the constructor (and, if parameter threaded is true, started) whereas it  probably ought at most to be created there and then started and stopped by the recovery manager start and stop calls. In the periodic recovery case this is done because in some circumstances the instance needs to be created without being started. Recovery runs as a background thread in the App Server but it can also be run via direct calls to scan(0 if the recovery manager is started in a separate recovery process outside the AS. So, this code is dual purpose.

What I have not yet ascertained is why the ExpiredEnrtyMonitor thread is not subject to the same condition. I think it probably needs to run in either case and since start does not get called when the recovery manager is  in its own process this is a cheap hack to make sure it always runs.

Anyway, as far as shutting down is concerned I can make one very clear statement. It is almost never appropriate to use a finalizer to clean up state. The only excuse for using finalizers is when a library hands out a reference to an instance to client code and cannot reasonably expect the client code to call a cleanup method to release resources held by the instance (n.b.in my book any respectable client program ought to guarantee to release any resources it allocates so this means a finalizer is never justified but sometimes you cannot pick and choose your clients). The reason it is such a bad thing to do is that finalizers make GC run like a pig. So it is best to avoid them at all costs.

Ok, that said, I believe it is currently valid to assume that when a recovery manager is stopped it will not be restarted. Anyway calling start after stop  wlll not work because the start method will try to restart the periodic recovery instance and calling start on a previously started thread is not valid. Anyway, given this assumption it makes no difference whether the ExpiredEnrtryMonitor is started in the constructor or in method start. So, the place to shut down the ExpiredEntryMonitor thread is in RecoveryManagerImple.stop.


> Shutdown the ExpiredEntryMonitor when stopping the RecoveryManager
> ------------------------------------------------------------------
>
>                 Key: JBTM-503
>                 URL: https://jira.jboss.org/jira/browse/JBTM-503
>             Project: JBoss Transaction Manager
>          Issue Type: Feature Request
>      Security Level: Public(Everyone can see) 
>          Components: Recovery
>    Affects Versions: 4.5
>         Environment: Tomcat + Spring + DBCP + JBossJTA
>            Reporter: Mauro Molinari
>            Assignee: Andrew Dinn
>
> When the recovery manager is started, the implementation RecoveryManagerImple starts the ExpiredEntryMonitor.
> When you stop the recovery manager, however, the ExpiredEntryMonitor is not shut down.
> On line 250 of com.arjuna.ats.internal.arjuna.recovery.RecoveryManagerImple there's the line ExpiredEntryMonitor.shutdown() commented out, with a "TODO why?" comment immediately before.
> I don't know exactly what should be the lifecylce of ExpiredEntryMonitor versus that of RecoveryManagerImple, however I think that one of the following should happen:
> - ExpiredEntryMonitor is started when RecoveryManagerImple is started (rather than constructed), so that it is shut down when the RecoveryManagerImple is stopped
> OTHERWISE
> - RecoveryManagerImple provides a way to "dispose" or "disable" it and this includes the shutdown of ExpiredEntryMonitor
> OTHERWISE
> - it is documented that stopping the recovery manager needs the ExpiredEntryMonitor to be stopped too: however, please consider that com.arjuna.ats.internal.arjuna.recovery.ExpiredEntryMonitor.shutdown() throws a NullPointerException if called while the ExpiredEntryMonitor has not been started before and there is no public method to test if it has been started or not; so if I need to write some code that stops the ExpiredEntryMonitor (without knowing if it has been started or not) I have to write some ugly code like:
> {code:java}
> try
> {
>   ExpiredEntryMonitor.shutdown();
> }
> catch(final NullPointerException e)
> {
>   // ExpiredEntryMonitor had not been started
> }
> {code}

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators: https://jira.jboss.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira