Introduce QuarkusBindException #23186
Conversation
This exception is used to record the actual ports that cause binding conflicts, thus allowing Quarkus to properly inform the user of which port(s) caused conflicts instead of guessing based on what the provided configuration. Fixes: quarkusio#22967
| Throwable effectiveCause = event.cause(); | ||
| if (effectiveCause instanceof BindException) { | ||
| if ((sslConfig == null) && (httpServerOptions != null)) { | ||
| effectiveCause = new QuarkusBindException(httpServerOptions.getPort()); |
There was a problem hiding this comment.
How would that work for negative ports (negative ports in Vert. x are random ports but all the verticles use the same port, unlike the port 0 which is random every time). I guess it's fine as it would indicate the negative port (like -1) and it's up to the user to check in the config which one it is.
There was a problem hiding this comment.
Yeah, although this is an edge case which I am going to guess we won't see often, if ever :)
I think we would probably just handle it when printing the helper messages if the exception fails.
Yes absolutely. This can and should be extended further |
geoand
added
the
triage/waiting-for-ci
|
|
||
| public QuarkusBindException(List<Integer> ports) { | ||
| if (ports.isEmpty()) { | ||
| throw new IllegalStateException("ports must not be empty"); |
There was a problem hiding this comment.
Hum, this is surely very unexpected, but wouldn't it be confusing for the user if he got that instead of e.g. Port ? seems to be in use by another process. (note the ? instead of the actual port)?
Might be better to add this as a suppressed exception?
There was a problem hiding this comment.
I just realized that suppression doesn't work as the exception is not (fully) logged. So maybe just a warning?
IDK, really just a minor thing...
There was a problem hiding this comment.
The reason I added this check is that I wanted to guard against erroneous usage and not have to deal with empty ports when inspecting the exception
There was a problem hiding this comment.
Sure, that makes sense. But if it really kicks in and a user is faced with it, then it will be more puzzling.
I guess in the end it's more of a "quarkus developer" guard than anything that will ever pop up for an actual quarkus user.
There was a problem hiding this comment.
Yeah, exactly! It's meant to be used by Quarkus developers (as it's part of the runtime package, so not meant to be part of the public API)
|
I won't backport this tonight, let's keep it for 2.7.1.Final. |
quarkus-bot
bot
removed
the
triage/waiting-for-ci
This exception is used to record the actual ports that
cause binding conflicts, thus allowing Quarkus to properly
inform the user of which port(s) caused conflicts
instead of guessing based on what the provided configuration.
Fixes: #22967
P.S. We could probably extend this even more in the future to provide the configuration property involved as well as the name of the "service" that tried to bind