"Missing" break in listenerStart?

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

"Missing" break in listenerStart?

Romain Manni-Bucau
Hi all,

I suspect it is intended but if so I wonder if it needs some toggle to
disable that behavior: is the fact to not break when a listener (start)
fails intended? ([1])

An ASF friend hit that with 2 listeners and second one was failling after
first one failed because it was depending on it.

Since this is not uncommon I wonder if it should get a break once ok=false
(issue can be listenerStop which should probably be independent of start
chain behavior since some listener only impl it) or if we should have a
flag in StandardContext to stop at first start failure.

Anything already thought on it?

[1]
https://github.com/apache/tomcat/blob/master/java/org/apache/catalina/core/StandardContext.java#L4669

Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> |  Blog
<https://rmannibucau.metawerx.net/> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
<https://www.packtpub.com/application-development/java-ee-8-high-performance>
Reply | Threaded
Open this post in threaded view
|

Re: "Missing" break in listenerStart?

remm
On Tue, Jan 12, 2021 at 5:49 PM Romain Manni-Bucau <[hidden email]>
wrote:

> Hi all,
>
> I suspect it is intended but if so I wonder if it needs some toggle to
> disable that behavior: is the fact to not break when a listener (start)
> fails intended? ([1])
>
> An ASF friend hit that with 2 listeners and second one was failling after
> first one failed because it was depending on it.
>
> Since this is not uncommon I wonder if it should get a break once ok=false
> (issue can be listenerStop which should probably be independent of start
> chain behavior since some listener only impl it) or if we should have a
> flag in StandardContext to stop at first start failure.
>
> Anything already thought on it?
>

For all other subcomponents of the context, the behavior is: set ok to
false, log the error and continue. It should stay that way. However, since
a ServletContextListener is a Servlet API component, then the Servlet
specification is supposed to resolve this one way or the other, but I don't
think it does. In a similar case the language is "log and fail to deploy".
As this is application related it could be reasonable to stop there.

Rémy

Reply | Threaded
Open this post in threaded view
|

Re: "Missing" break in listenerStart?

Romain Manni-Bucau
Drafted https://github.com/apache/tomcat/pull/400 in regards of that topic.

Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> |  Blog
<https://rmannibucau.metawerx.net/> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
<https://www.packtpub.com/application-development/java-ee-8-high-performance>


Le mar. 12 janv. 2021 à 23:51, Rémy Maucherat <[hidden email]> a écrit :

> On Tue, Jan 12, 2021 at 5:49 PM Romain Manni-Bucau <[hidden email]>
> wrote:
>
> > Hi all,
> >
> > I suspect it is intended but if so I wonder if it needs some toggle to
> > disable that behavior: is the fact to not break when a listener (start)
> > fails intended? ([1])
> >
> > An ASF friend hit that with 2 listeners and second one was failling after
> > first one failed because it was depending on it.
> >
> > Since this is not uncommon I wonder if it should get a break once
> ok=false
> > (issue can be listenerStop which should probably be independent of start
> > chain behavior since some listener only impl it) or if we should have a
> > flag in StandardContext to stop at first start failure.
> >
> > Anything already thought on it?
> >
>
> For all other subcomponents of the context, the behavior is: set ok to
> false, log the error and continue. It should stay that way. However, since
> a ServletContextListener is a Servlet API component, then the Servlet
> specification is supposed to resolve this one way or the other, but I don't
> think it does. In a similar case the language is "log and fail to deploy".
> As this is application related it could be reasonable to stop there.
>
> Rémy
>
>
> >
> > [1]
> >
> >
> https://github.com/apache/tomcat/blob/master/java/org/apache/catalina/core/StandardContext.java#L4669
> >
> > Romain Manni-Bucau
> > @rmannibucau <https://twitter.com/rmannibucau> |  Blog
> > <https://rmannibucau.metawerx.net/> | Old Blog
> > <http://rmannibucau.wordpress.com> | Github <
> > https://github.com/rmannibucau> |
> > LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
> > <
> >
> https://www.packtpub.com/application-development/java-ee-8-high-performance
> > >
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: "Missing" break in listenerStart?

Konstantin Kolinko
Hi!

ср, 13 янв. 2021 г. в 17:04, Romain Manni-Bucau <[hidden email]>:
>
> Drafted https://github.com/apache/tomcat/pull/400 in regards of that topic.
>

Looks good thus far.
Several notes:

1. Maybe others disagree, but I am OK with the default for
"exitOnFirstListenerFailure" property being changed to "true".

It is a rare use case, and I think that it is safe to fail early. The
web application will fail to start anyway.

2. A better name?

I do not like "exit**", because Tomcat does not exit here. It does not
call "System.exit()" or anything like that. It is just a single web
application that fails to start.

Maybe something like "abortOnFirstListenerFailure".

(There is org.apache.catalina.startup.EXIT_ON_INIT_FAILURE system
property that really forces Tomcat as a whole to shut down. Thus
"exit" is used in its name.)

3. The attribute has to be added to the JMX configuration file,
java/org/apache/catalina/core/mbeans-descriptors.xml

4. The attribute has to be documented in the Configuration Reference,
webapps/docs/config/context.xml

5. Looking for other similar listeners and callbacks.

E.g. ServletContainerInitializers

            for (Map.Entry<ServletContainerInitializer, Set<Class<?>>> entry :
                initializers.entrySet()) {
                try {
                    entry.getKey().onStartup(entry.getValue(),
                            getServletContext());
                } catch (ServletException e) {
                    log.error(sm.getString("standardContext.sciFail"), e);
                    ok = false;
                    break;
                }
            }

Looking at implementations of org.apache.catalina.LifecycleListener -
they are OK.
(Those listeners are called notified via
"org.apache.catalina.util.LifecycleBase.fireLifecycleEvent()". That
method does not catch exceptions and thus fails early.)

Best regards,
Konstantin Kolinko

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: "Missing" break in listenerStart?

Romain Manni-Bucau
Le mer. 13 janv. 2021 à 15:42, Konstantin Kolinko <[hidden email]>
a écrit :

> Hi!
>
> ср, 13 янв. 2021 г. в 17:04, Romain Manni-Bucau <[hidden email]>:
> >
> > Drafted https://github.com/apache/tomcat/pull/400 in regards of that
> topic.
> >
>
> Looks good thus far.
> Several notes:
>
> 1. Maybe others disagree, but I am OK with the default for
> "exitOnFirstListenerFailure" property being changed to "true".
>
> It is a rare use case, and I think that it is safe to fail early. The
> web application will fail to start anyway.
>

Any (easy) way to rerun the TCK to validate this does not break anything
before?


>
> 2. A better name?
>
> I do not like "exit**", because Tomcat does not exit here. It does not
> call "System.exit()" or anything like that. It is just a single web
> application that fails to start.
>
> Maybe something like "abortOnFirstListenerFailure".
>
> (There is org.apache.catalina.startup.EXIT_ON_INIT_FAILURE system
> property that really forces Tomcat as a whole to shut down. Thus
> "exit" is used in its name.)
>

We can also reverse it like "alwaysInitializeAllContextListeners", can be a
way to make it more explicit.


>
> 3. The attribute has to be added to the JMX configuration file,
> java/org/apache/catalina/core/mbeans-descriptors.xml
>

Will do once we all agree on a name.


>
> 4. The attribute has to be documented in the Configuration Reference,
> webapps/docs/config/context.xml
>

The same I guess ;).


>
> 5. Looking for other similar listeners and callbacks.
>
> E.g. ServletContainerInitializers
>
>             for (Map.Entry<ServletContainerInitializer, Set<Class<?>>>
> entry :
>                 initializers.entrySet()) {
>                 try {
>                     entry.getKey().onStartup(entry.getValue(),
>                             getServletContext());
>                 } catch (ServletException e) {
>                     log.error(sm.getString("standardContext.sciFail"), e);
>                     ok = false;
>                     break;
>                 }
>             }
>

There is already a break there so no need of any update I guess?


>
> Looking at implementations of org.apache.catalina.LifecycleListener -
> they are OK.
> (Those listeners are called notified via
> "org.apache.catalina.util.LifecycleBase.fireLifecycleEvent()". That
> method does not catch exceptions and thus fails early.)
>
> Best regards,
> Konstantin Kolinko
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>