Test config creation sometimes delegates to instance variable and sometimes to superclass, causing (v v minor) chaos

[holly-cummins]

I just spent a while wondering if I was losing my mind, until the debugger helped me out.

I was staring at code in the TestConfigProviderResolver that did this:

                resolver.releaseConfig(classLoader);
                resolver.registerConfig(config, classLoader);

... and it was failing because the config hadn't been released. "How can it not be released? I can see the release call one line above the failing call, and there's no concurrency here!" I wailed.

Working through the problem in a debugger, I realised that the release and register call were being applied to different objects. At this point, I was hollering at the computer "What? how they be different objects? They're the same object, I can see it right there!"

The wrinkle in this case, is that TestConfigProviderResolver both extends from another resolver, and holds an instance of it as an instance variable. The superclass and instance variable are not at all guaranteed to be the same object. In this case, one method was going to the superclass, and one was going to the instance variable. I could only spot that by staring at screencaps of the debugger on the release and register calls.

It would be nice if TestConfigProviderResolver could implement an interface instead, since that avoids this kind of risk, but I assume that's not possible.

[quarkus-bot]

Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?

• title should not end up with ellipsis (make sure the title is complete)

_{This message is automatically generated by a bot.}

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit c1c3bdf.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

[gsmet]

Let's backport to 3.18.

And yes I would be in favor of having an interface and a delegation model.

[radcortez]

Yes, the problem here is that I needed specific methods from SmallRyeConfigProviderResolver, but these are not available in the MP Config interface ConfigProviderResolver, so to move forward I did it that way :)

I'll improve it.

[holly-cummins]

I agree we should backport, since the change is (a) obviously correct and (b) low-risk. But I also think this is a very latent defect; as far as I know, we weren't seeing any symptoms until I started pushing the test classloading down new paths.

Probably the next time the current design would cause problems is if a new method gets added to the superclass and we don't remember to update the subclass. That's a risk, but not an immediate one. Improvement would be good, but I'd say it's definitely not the highest priority.