Make sure hot deployment works after a failed server start in an application with non-default http port

[jaikiran]

This should fix the issue reported in #4815

This includes 2 commits - one which fixes the parsing of the configuration options in ConfigInstantiator which was resulting in not using the configured port in case of failed start. This ConfigInstantiator is only used in this specific failed server start case and for limited use case and thus explains why such a basic issue wasn't noticed in this one so far.
The second commit in this PR includes a change in VertxHttpRecorder to make sure the hot deployment handler is setup even after the server failed to start.

Given the nature of this change, there isn't any easy way to add a test to this one. I have manually tried a bunch of use cases with this change. That includes, using non-default http port and then starting Quarkus such that it fails during startup and then updating the application.propreties to make sure the change fixes the startup and then issuing a request to it. Went fine. Have also tried the usual successful cases and those too went fine.

[emmanuelbernard]

For the first commit, what you did is remove Config/Configuration before dashifying, right? Is that what failed before because the dashification was hidding the capitalized C in the string replacement? I have to admit I have a hard time following the core of the changes.

For second commit, it's not make sure the proper set of files are listened to, correct?

[emmanuelbernard]

Do you mind avoiding using final on local variables. I don't think we use that approach elsewhere and in your case it shows artificial line changes compared to the original code which makes it harder to read.

[jaikiran]

@emmanuelbernard, I'll keep that in mind for unchanged lines going forward.

[emmanuelbernard]

@jaikiran I tested it but it seems to still only react on port 8080 upon failure.

1. create a project with panache and postgresql but zero config
2. set quarkus.http.port=8081
3. create Todo with @entity on it
4. mvn clean ; mvn compile quarkus:dev
5. cannot reach localhost:8081 but can reach localhost:8080

So the patch does not seem to address the problem

[Mefl]

@emmanuelbernard I am surprised by your comment, it tested it and it worked on my case.

The curl on localhost:8082 triggered the reload and the result was good.
On third test the file change triggered the reload itself before I had the time to send the curl request.

[emmanuelbernard]

@Mefl arg weird. I did build from the branch, damn. Let me do a clean one.
BTW @Mefl we never reload without a HTTP being hit. So what you saw as reloaded before curl is fishy

[emmanuelbernard]

Year I am seeing the same results even after rm -fR .me/repository/io/quarkus

What I did is

git fetch origin
git  checkout -b 4934 upstream/pr/4934
rm -fR ~/.m2/repository/io/quarkus/
mvn clean install -DskipTests

and on an new app did the steps I mentioned above.

And boom only reacts to 8080

[emmanuelbernard]

Maybe @FroMage you could be the judge, is that me scrambling something or is the PR having an issue?

[stuartwdouglas]

I tested this locally, it works for me.

[stuartwdouglas]

@emmanuelbernard you are using 999-SNAPSHOT in the app you are testing right?

[stuartwdouglas]

@emmanuelbernard I am just going to merge this, as we have 3 others confirming that is fixes their problem (and it is clearly fixing a real problem). If you still have problems they are likely something different.

[geoand]

We must be testing different things because when I followed @emmanuelbernard's scenario, I observed the exact same behavior he did: The application was listening on 8081 instead of 8080.

[stuartwdouglas]

@geoand 8081 is the correct behaviour.

[geoand]

@stuartwdouglas sorry, in my comment above I meant to say that 8080 was working and 8081 was not.

[emmanuelbernard]

@stuartwdouglas if you feel it’s fixing something sure. Push. I’ll try and reproduce with a clean scenario with another issue tomorrow.

[stuartwdouglas]

#4950

So basically there was different behaviour depending on if the failure happened during augment or runtime. Runtime sets config properties globally, so application.properties was still available. If it failed in augment it was only attached to the current CL, which was not present when the startup was happening

[stuartwdouglas]

So to reproduce this you needed to both use application.properties rather than a system property, and pick a failure that would be reported at build time rather than runtime.

[jaikiran]

Sorry @emmanuelbernard, after I submitted this PR, I just called it a day and am only now checking these messages.

For the first commit, what you did is remove Config/Configuration before dashifying, right? Is that what failed before because the dashification was hidding the capitalized C in the string replacement? I have to admit I have a hard time following the core of the changes.

Yes, that was one part of the reason why hot deployment wasn't kicking in correctly. What was happening was the logic in ConfigInstantiator#handleObject(Object o) was incorrect. This method is getting used in VertxHttpRecorder#startServerAfterFailedStart and it's being passed a new instance of HttpConfiguration as a param. This param is then supposed to be populated using the configured properties in the application.properties. What was happening was, the logic in that ConfigInstantiator#handleObject(Object o) had a couple of issues:

1. It only recognized classes whose name ended with Config. So since HttpConfiguration doesn't end with Config, it was ending up computing a wrong property prefix to lookup in the application.properties. As a result, properties like quarkus.http.port weren't being honoured when this object was being populated after a failed start of the server. That caused the failed start logic to use default port 8080 and start listening on that port.
2. The other issue in this ConfigInstantiator class was the implementation was the dashify method. If it was passed something like FooBar as a param to dashify, it was ending up returing -foo-bar instead of foo-bar. So instead of looking for quarkus.http.port, it would end up looking for quarkus.-http.port

So the first commit in this PR fixed these 2 issues.

For second commit, it's not make sure the proper set of files are listened to, correct?

The second commit in the PR involves the VertxHttpRecorder.startServerAfterFailedStart() method. This gets called when the start fails. This method first shuts down dev mode which involves resetting the hot deployment handler to null and then setting up a HTTP server to listen to further requests. The problem was that since the hot deployment handler was set to null, even though the HTTP server is now up and running waiting for requests, it no longer had the hot deployment handler to check for changes in watched files. The 2nd commit in this PR fixed this issue by making sure that after the dev mode shutdown logic in this method, it will continue to hold on to the previously setup hot deployment handler, so that it can be used to watch the changed files (one of them being the application.properties).