Implemented addBeanFromConstructor

[gregw]

Added mechanism to safely add beans from a super constructor of ContainerLifeCycle

Fixes #11317

[lorban]

I find some elegance in this proposal, the only things I'm reserved about are:

• the addBeanFromConstructor method name is horrible, how about installBean instead?
• maybe objects added with that new method should be impossible to remove with removeBean?
• we should add a test to make sure addBeanFromConstructor doesn't make calls on the object-under-construction's reference, for instance by calling toString on it because of a logging call.
• I'm reserving my final judgment for when all tests will be passing without any bad surprise.

[gregw]

I find some elegance in this proposal, the only things I'm reserved about are:

• the addBeanFromConstructor method name is horrible, how about installBean instead?

OK

• maybe objects added with that new method should be impossible to remove with removeBean?

We spend a lot of effort to make sure beans are kept up to date with update mechanisms, so I don't think this is needed.

• we should add a test to make sure addBeanFromConstructor doesn't make calls on the object-under-construction's reference, for instance by calling toString on it because of a logging call.

working on it

• I'm reserving my final judgment for when all tests will be passing without any bad surprise.

I've fixed some beans that were listeners, but I expect there will be a few more.

[lorban]

While I think this change has merits, it does not address the original problem reported in #11220 as in that case, the cause was:

• Handler$Wrapper's constructor
  • calls Handler$Singleton.updateHandler
    • calls ContainerLifeCycle.updateBean
      • calls ContainerLifeCycle.addBean

so addBean may be called indirectly from a general-purpose method, which bypasses this constructor-specific method.

[lorban]

Also, the following test fails when DEBUG logs are enabled, despite your change:

    @Test
    public void testInstallBeanDoesNotEscapeConstructor()
    {
        ConcreteClazz c = new ConcreteClazz();
        assertThat(c.exception, is(nullValue()));
    }

    private static abstract class AbstractClazz extends ContainerLifeCycle
    {
        public AbstractClazz()
        {
            // Pass a reference to a half-constructed object to installBean.
            installBean(this);
        }
    }

    private static class ConcreteClazz extends AbstractClazz
    {
        private final List<String> aList = List.of("single element");

        private NullPointerException exception;

        @Override
        public String toString()
        {
            try
            {
                // Call a method on the aList field, if there was a constructor escape the ref will be null despite being final.
                String entry = aList.get(0);
                return "ConcreteClazz@" + hashCode() + "[" + entry + "]";
            }
            catch (NullPointerException npe)
            {
                exception = npe;
                throw npe;
            }
        }
    }

[gregw]

@lorban I think both of your examples are poor code:

• The Handler.Wrapper should not be calling updateHandler from a constructor, as there is no handler to update. I've factored out the sanity checks to a checkHandler method instead so now installBean can be used.

• In the second example, I don't think it is valid to add a bean to itself. Any constructor that passes this out in a method is vulnerable like that code. But correct usage of installBean does not need this to be passed out.

[gregw]

@sbordet @lorban nudge

[gregw]

@sbordet @lorban nudge again

[gregw]

@sbordet @lorban nudge again again

[lorban]

We need some test coverage to make sure we don't de-reference objects under construction, otherwise LGTM.

[lorban]

The #1 motivation for this new installBean method was to make sure we don't de-ref objects under construction. We should find a way to test that.

[gregw]

Neither me nor chatGPT can think of a way that the method addBean can tell if it has been called from a super constructor or not. Thus it is impossible for us to test if it is safe for addBean to call a listener passing this. We have provided and alternative to addBean that is safe to call from constructors and we have javadoc'd that alternative.

I have visually scanned the list of callers of addBean and can see no constructors in there. However, I cannot tell if any of those callers are themselves called from a constructor.

I doubt that we are perfect.... but we are better than we were!

[lorban]

I believe we should have added a test like the following as part of #11321. It is up to you to include it in this PR or not, as that is the best coverage I can come up with, and I agree it's not much.

    @Test
    public void testHandlerWrapperDoesNotLogUnconstructedObject()
    {
        JettyLogger jettyLogger = (JettyLogger)LoggerFactory.getLogger(ContainerLifeCycle.class);
        JettyLevel originalLevel = jettyLogger.getLevel();
        PrintStream originalPrintStream = System.err;
        try
        {
            jettyLogger.setLevel(JettyLevel.ALL);
            ByteArrayOutputStream baos = new ByteArrayOutputStream();
            PrintStream printStream = new PrintStream(baos);
            System.setErr(printStream);

            new Handler.Wrapper(new EchoHandler())
            {
                private final List<String> strings = List.of("aString");

                @Override
                public String toString()
                {
                    // De-ref the final field here.
                    return "string[0]=" + strings.get(0);
                }
            };

            printStream.flush();
            String output = baos.toString(StandardCharsets.UTF_8);

            // Check that SLF4j did not print an error to stderr.
            assertThat(output, allOf(not(containsString("SLF4J: Failed toString()")), not(containsString("NullPointerException"))));
        }
        finally
        {
            jettyLogger.setLevel(originalLevel);
            System.setErr(originalPrintStream);
        }
    }

[gregw]

I believe we should have added a test like the following as part of #11321. It is up to you to include it in this PR or not, as that is the best coverage I can come up with, and I agree it's not much.

Maybe as a full integration test with almost all our modules turned on, full debug and a few requests sent in, then we check the log for anything that should not be there.... But I think if it is just a few handlers, it does not test much.

[gregw]

@sbordet nudge!