Cleanup usages of addBean(Object) from constructors

[joakime]

Jetty version(s)
12

Jetty Environment
All

Java version/vendor (use: java -version)
All

OS type/version
All

Description
In the process of working Issue #11220 and the PR #11225 it was discovered that we have a problem with calling virtual methods (like addBean(Object)) from constructors where we can wind up with partially initialized classes that are then used / referenced / passed around in different ways.

One is the DEBUG logging of addBean with a parameter of this ...

jetty.project/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/component/ContainerLifeCycle.java

Lines 418 to 419 in 7fbd51a

	if (LOG.isDebugEnabled())
	LOG.debug("{} added {}", this, newBean);

Another is the call to the bean added listeners with this parameter during addBean.

jetty.project/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/component/ContainerLifeCycle.java

Lines 348 to 350 in 7fbd51a

	// Tell any existing listeners about the new bean
	for (Container.Listener l : _listeners)
	l.beanAdded(this, o);

We should strive to move to a doStart() > addBean() and later call to doStop() > removeBean() (like what is being done in fc32100

[joakime]

CC @gregw @lorban @sbordet

[gregw]

I'd be very cautious about such a change. Often beans are used in configuration, which is done before a doStart call.

[joakime]

I'd be very cautious about such a change. Often beans are used in configuration, which is done before a doStart call.

If the method call addBean(Object) is used in a constructor for those configurations then it's an issue.
If the beans are manipulated in any other capacity outside of the constructor and before doStart then it's not an issue.

[joakime]

The heart of the issue is the passing of this from within the method addBean() to various places.
That reference to this is not yet fully constructed (eg: the fields on this are not yet initialized), if addBean(Object) is used during within a constructor.

We (@sbordet @lorban and I) did a quick review of this scenario in the Jetty 12 codebase.
Surprisingly we do this a lot in jetty-client code (several dozen places), and not much outside of jetty-client code (found only 3 places so far).

[gregw]

The object passed to addBean should be fully constructed, so that should not be an issue. References to this by AbstractContainer are a bit more problematic, but they at least should only be seeing the AbstractContainer aspect of the object... unless a virtual method (toString) is called.

Perhaps the better "solution" is to move all the addBean calls to be the last thing done in the constructors rather than early on. Not a perfect solution, but we have been adding beans from constructors forever.

[joakime]

Still won't be fully constructed on abstract usage and calls to super() from a constructor.
Like we have in ContextHandler -> Handler.Wrapper.
The addBean() call in Handler.Wrapper occurs before the ContextHandler is fully constructed.

[gregw]

I understand the issue. But the proposed cure is worse than the disease.

Potentially we should deprecate the newly added convenience constructor that passes in a Handler and make an explicit setHandler or addHandler call instead after construction.
We can also move addBean calls to be last in the constructor.

If we still have issues, then we can add a protected addBeanFromConstructor(Object) that does not call the listeners, followed by an explicit call to call the listeners, that would at least be done from doStart , but could be triggered by a virtual method called from the constructor if need be.

[gregw]

If we had an protected void addBeanFromConstructor(Object) in ContainerLifeCycle, then it could avoid calling any listeners, because there should be none registered yet.... unless an object passed to addBeanFromConstructor was itself an EventListener. Perhaps we could throw IAE in that case?

[gregw]

@joakime @sbordet see #11319 for a speculative fix for this issue.

[lorban]

I think @gregw made a few good points: calling addBean from constructors has been the norm forever; we use that pattern extensively, and users may do too.

Adding a new method like addBeanFromConstructor that makes sure this doesn't escape looks like a reasonable middle-ground: refusing to call listeners and making sure we do not dereference the object-under-construction's reference would solve the problem while keeping most of addBean's goodness.

It's still a behavioral change (maybe someone relies on the listeners being called?), but IMHO that is a small sacrifice.

[joakime]

How about a smaller fix?

See PR #11321

[sbordet]

I would go simpler and just fix the logging statement and clarify the javadoc that addBean() should not be called from the constructor.

Listeners receive a half-constructed container when addBean() is called from the constructor, and it has been so for a while, but apparently nobody noticed.

I can live with installBean(), but then we need another method to call the listeners that installBean() does not call from doStart(), at which point you can just call addBean() from doStart().

[gregw]

@sbordet during construction, there should be no listeners that installBean needs to call. I've only found one case where an installed bean was itself a listener and it was an easy work around.

I'm not 100% convinced we need installBean, but I'll work on the PR and we can see what it looks like.

There fix on toString not using this does 95% of what is needed