Always start dev services before getting the dev services properties

[holly-cummins]

Resolves #49208

This builds on @vonatzigenc's fix, and generalises it. When calling code asks for dev services properties, we should always make sure dev services are started by that point. If nothing asks for those properties, we should start dev services as the first step of the startup sequence.

This does have the ickiness that the getter method has side effects, and it also means it's a bit loosely-defined when dev services start. The alternative was to have calling code explicitly request dev services be started before calling the getter. On balance, I decided that was more risky.

I was surprised that we didn't catch this, since the original code broke a quickstart. I haven't investigated that yet, to see if there's anything we can do, but I did add a couple of tests in the main repo which show the problem.

[Sanne]

no strong objections, but I'd be a bit concerned that going forward we might have situations in which some code (inadvertently or not) triggers starting of services which aren't actually necessary? That could become a painful UX, and not necessarily propagate the feedback to us.

What about throing an exception if the getter is triggered but the state of the services isn't valid? It might be more irky to develop with, but we'd benefit from such guidance during development of extensions, or worst case we'd get actionable bug reports.

[holly-cummins]

no strong objections, but I'd be a bit concerned that going forward we might have situations in which some code (inadvertently or not) triggers starting of services which aren't actually necessary? That could become a painful UX, and not necessarily propagate the feedback to us.

I know what you mean, but I think that's not too much of a concern. In the old model, all dev services would have been started in the augmentation phase. In the new model, all dev services get started as the first step of run(). So in the old model, we always would have started dev services before the getDevServicesProperties method was available. In the new model, you'd only get unnecessary dev services start if an augmentation was run without any intention of starting an application (which is kind of weird anyway), and then after running the augmentation, calling code wanted to know dev services properties that could only be read after dev service startup, but wanted all the dev services to stay stopped. In that situation, I think the calling code deserves what it gets. :)

What about throing an exception if the getter is triggered but the state of the services isn't valid? It might be more irky to develop with, but we'd benefit from such guidance during development of extensions, or worst case we'd get actionable bug reports.

Developers of extensions shouldn't be calling the method on StartupAction, I don't think (although it may be exposed to them). So the complexity worry is more than we have about five different start paths internally, all subtly different. Making it harder to reliably start Quarkus (by forcing calling code to explicitly start dev services as well as starting the application) increases the risk of things falling through our testing and getting into situations where on some paths, dev services that should have been started weren't started.

[Sanne]

thanks Holly - that all makes sense, looks like my mental model of the internals is way out of date :)

Still it irks me a bit that an "innocently looking getter" would have such a wild side-effect as e.g. having to wait minutes to start some heavy weight DBs - wondering if that should be made more explicit.

[holly-cummins]

Still it irks me a bit that an "innocently looking getter" would have such a wild side-effect as e.g. having to wait minutes to start some heavy weight DBs - wondering if that should be made more explicit.

Yeah, I know what you mean. I wondered about something like a method called initialiseDevServicesProperties which returns the properties, and initialiseDevServicesNetworkId to return the properties. But then that's a bit confusing for the case where they're no-op getters (and one of those two methods always would be). So it could get be getOrInitialiseDevServicesProperties and getOrInitialiseDevServicesNetworkId, which is correct, but awkwardly verbose. But maybe correct is the most important thing. WDYT?

[Sanne]

But maybe correct is the most important thing.

+1

[holly-cummins]

But maybe correct is the most important thing.

+1

Ok, done! The other disadvantage is that it makes the changeset bigger and changes an interface, but that should be ok.

[quarkus-bot]

Status for workflow Quarkus CI

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

✅ 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.