Only stop subcomponents that were started directly.

[philsttr]

Each component in logstash-logback-encoder now keeps track of which subcomponents it started, and only stops those that it started directly.

This change allows the lifecycle of subcomponents to be managed outside of the components that use them (e.g. for programmatic configuration, and reuse of subcomponents)

This change has a major side effect when xml configuration is used. Since logback automatically starts components created from xml configuration, the logstash-logback-encoder components never start their subcomponents that were configured via xml, which means that logstash-logback-encoder components will never stop their subcomponents that were configured via xml. Additionally, logback does NOT stop all components that it started when the logback context is stopped. It only stops Appenders and TurboFilters. This means that any components configured via xml that are NOT Appenders/TurboFilters will NOT be stopped.... ever. IMHO, this is a bad design in logback... if logback starts a component, it should stop it on shutdown, but that's not the case.

Related to #473

[brenuart]

I like the idea of a LifecycleManager to keep track of which components had to be started.

However, the more I think about it, the more it seems to me that start() usually performs two distinct operations:

• tells the component that all its properties have been set and that it can validate its configuration
• actually start the component - it can create intermediate helper objects, acquire resources, start threads, etc
  This should ideally have been modelled by two distinct methods...

The question raised in #473 concerns essentially the subcomponents that are injected via setters (or constructor). I think you would agree that if a components creates sub-components itself, then it must manage their lifecycles.

It should not harm to call start() multiple times. This means that a component may start() sub-components (owned or not) without any problem - even if it does not check for isStarted(). And I would say that it is even very practical because in this way we have the guarantee they are started without relying on the "caller" to do it....

The real question is whether they should be stopped or not.. The more I think about it, the more I believe that sub-components should NOT be stopped. Why? Stopping a sub-components means you take full control on its lifecycle that is now bound to the lifecycle of your component. This is an important limitation which prevents the reuse of the same component to inject it into several others. Think about a LoggingEventCompositeJsonEncoder that is injected into a FileAppender and a TcpAppender. If you stop the TcpAppender at runtime, the encoder is stopped as well and may not be usable by the FileAppender anymore...

I'm puzzled... I don't think there is a solution for this - its a design flaw in Logback. The best we can do is to be consistent in how our components behave and adopt the safest approach which is IMHO:

• start sub-components so they can validate their configuration and you don't get exceptions at runtime when the first event is received. Best practice is to check the StatusManager after initialisation of the logging system is completed and raise an exception when it contains Error statuses.
• don't stop sub-components unless you own them (their lifecycle is bound to yours). This allows for reuse of the same instance by other components. If some need an explicit stop, then... well, users will have to take care of that themselves...

(this conclusion seems the opposite of what I proposed in #473... sorry about that... I slightly changed my mind after reviewing how and why we configured everything in our applications)

[philsttr]

I think the current implementation is "compatible" with both your original idea in #473 (approach 1) and your latest comment here (approach 2).

For example, with the code in this PR, if you programmatically configure the same encoder to be used by multiple appenders, then you can tell the appenders that they don't "own" the lifecycle of the encoder by starting the encoder before starting either appender. This seems intuitive to me.

I generally like the "if you start a component, you own that component's lifecycle" approach, because it's really easy to explain and comprehend. It's unfortunate that logback only follows this approach for Appenders/TurboFilters when xml configuration is used.

Having said that, if logstash-logback-encoder implements either approach, it would be a change that logstash-logback-encoder components no longer stop non-encapsulated subcomponents, either because the subcomponents were started by logback (approach 1), or because stopping of non-encapsulated subcomponents is removed completely (approach 2). After thinking more about this, I think this is ok.

[brenuart]

After thinking more about this, I think this is ok.

Give me until tomorrow to think about it once again (it is 1am already here)... Ok?