Bug 50836 - LifecycleState#isAvailable() to be more restrictive
Summary: LifecycleState#isAvailable() to be more restrictive
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 7
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 7.0.8
Hardware: PC Windows XP
: P2 normal (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-26 14:05 UTC by Konstantin Kolinko
Modified: 2011-02-27 09:36 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Konstantin Kolinko 2011-02-26 14:05:18 UTC
I think some of the values returned by LifecycleState#isAvailable() are wrong.

1. I am certain that MUST_DESTROY should return false, because it is a state that is later than stopped.
2. I think that STOPPING_PREP should return false, because we can get there from the FAILED state.
3. I think that STARTING should return false, because it is too early to return true there.

As a result, the following is the complete list of states are to return true in #isAvailable():
STARTED, MUST_STOP
Comment 1 Mark Thomas 2011-02-26 14:22:21 UTC
Agree with MUST_DESTROY.

Need to review the others.
Comment 2 Mark Thomas 2011-02-26 14:31:37 UTC
isAvailable() is aligned with when the public methods are available for use by other components. On that basis isAvailable() should return true for states STOPPING_PREP and STARTING. I'm sure there is scope to improve the documentation and MBean method descriptions to that effect.
Comment 3 Konstantin Kolinko 2011-02-26 14:54:47 UTC
(In reply to comment #2)
> are available for use by other components.

The "public" methods: I think those are methods that perform some work, like AccessLog#log(), not just any public methods.  Or you are talking about JMX here?

Regarding STARTING:

I think that in STARTING it is too early to call those methods:  the component has not completed its start up.

E.g, if AccessLogValve#startInternal() is overriden in some child class, STARTING state will be set by AccessLogValve#startInternal(), before the child class method completes.


Regarding STOPPING_PREP: 

My only concern is that the previous state might be FAILED. When transitioning FAILED -> STOPPING_PREP the availability flag suddenly becomes "true".
Comment 4 Mark Thomas 2011-02-26 15:34:28 UTC
(In reply to comment #3)
> The "public" methods: I think those are methods that perform some work, like
> AccessLog#log(), not just any public methods.  Or you are talking about JMX
> here?

I was quoting from the Lifecycle Javadoc. What is meant by public methods is not defined but I take it to mean "any public method that isn't a property setter/getter or a lifecycle methods". That could be better defined in the Lifecycle Javadoc.
 
> Regarding STARTING:
> 
> I think that in STARTING it is too early to call those methods:  the component
> has not completed its start up.

It should be OK. Do you have an example of where it isn't?

> E.g, if AccessLogValve#startInternal() is overriden in some child class,
> STARTING state will be set by AccessLogValve#startInternal(), before the child
> class method completes.

Providing the class follows the guidelines in the Javadoc for Lifecycle and LifecycleBase that should be fine.

> Regarding STOPPING_PREP: 
> 
> My only concern is that the previous state might be FAILED. When transitioning
> FAILED -> STOPPING_PREP the availability flag suddenly becomes "true".

I think we could safely move from FAILED to STOPPING in that case. It means changing the permitted Lifecycle transitions slightly but from a quick look at the code it should be ok.
Comment 5 Konstantin Kolinko 2011-02-26 19:15:00 UTC
(In reply to comment #4)
> > Regarding STARTING:
> > 
> > I think that in STARTING it is too early to call those methods:  the component
> > has not completed its start up.
> 
> It should be OK. Do you have an example of where it isn't?
> 

I do not have one. I was thinking about AccessLog in context of bug 50835 (AccessLog#log() being called on uninitialized valve in unclear circumstances in some broken configuration), but I do not have any custom implementation.

I am also concerned with StandardContext#getAvailable() delegating to LifecycleState#isAvailable().


If Child extends AccessLogValve, implementing Child#startInternal() to do not make the component available too soon essentially means that super.startInternal() should be called at the end of child's method, not sooner.

Transition to STARTING means that Lifecycle.START_EVENT will be fired after the transition, but we are not waiting for the listeners to complete before declaring the component "available".

Transition to STARTED is performed automatically in LifecycleBase#start(). All startup procedures for the component are completed in that state, and I think the STARTED state is better place to mark it as "available".

> > Regarding STOPPING_PREP: 
> > 
> > My only concern is that the previous state might be FAILED. When transitioning
> > FAILED -> STOPPING_PREP the availability flag suddenly becomes "true".
> 
> I think we could safely move from FAILED to STOPPING in that case. It means
> changing the permitted Lifecycle transitions slightly but from a quick look at
> the code it should be ok.

I like the idea. It means that we are not firing Lifecycle.BEFORE_STOP_EVENT, but that is not much to lose.
Comment 6 Mark Thomas 2011-02-27 05:49:00 UTC
(In reply to comment #5)
> I do not have one. I was thinking about AccessLog in context of bug 50835
> (AccessLog#log() being called on uninitialized valve in unclear circumstances
> in some broken configuration), but I do not have any custom implementation.

If the Javadoc is followed, I don't see how that bug happened. I'd be happy to revist this with a reproducible test case.

> I am also concerned with StandardContext#getAvailable() delegating to
> LifecycleState#isAvailable().

It is always possible to change the implementation of StandardContext#getAvailable() to check for a more limited set of states.

I count 46 references to isAvailable(). I can see at least one place where changing that will break something (ContainerBase.addChildInternal()) and I am concerned that there will be places that are missed if we make this change.

> If Child extends AccessLogValve, implementing Child#startInternal() to do not
> make the component available too soon essentially means that
> super.startInternal() should be called at the end of child's method, not
> sooner.

It depends. That exact requirements are defined in the Javadoc and I am in the process of making that Javadoc even more explicit.

> Transition to STARTING means that Lifecycle.START_EVENT will be fired after the
> transition, but we are not waiting for the listeners to complete before
> declaring the component "available".

I'm happy with that. The listeners should be fired after the state change. Again, I'll clarify this in the Javadoc.

> Transition to STARTED is performed automatically in LifecycleBase#start(). All
> startup procedures for the component are completed in that state, and I think
> the STARTED state is better place to mark it as "available".

"available" means whatever we define it to mean. I'm happy with the way the current code works but can see lots of places where the Javadoc can be improved.

> > I think we could safely move from FAILED to STOPPING in that case. It means
> > changing the permitted Lifecycle transitions slightly but from a quick look at
> > the code it should be ok.
> 
> I like the idea. It means that we are not firing Lifecycle.BEFORE_STOP_EVENT,
> but that is not much to lose.

That event can still be fired.

I have a patch for most of the above, I just need to finish some of the Javadoc improvements.
Comment 7 Mark Thomas 2011-02-27 06:02:09 UTC
I have applied a patch to 7.0.x that will be included in 7.0.9 that I believe addresses nearly all of the concerns raised in this issue. The bulk of the change clarifies what is meant by "available".

The outstanding issue is perhaps what StandardContext#getAvailable() returns but that is probably better discussed in a new issue / on the dev list if it is still of concern.
Comment 8 Konstantin Kolinko 2011-02-27 09:36:52 UTC
OK. I like it. Let's stop at this.