Downgrade stopServiceResolution listener-not-registered to a warning

[agners]

Summary

On API 34+ the NSD framework auto-unregisters the resolve listener once onServiceResolved/onResolveFailed fires, so the unconditional stopServiceResolution call in awaitClose throws IllegalArgumentException: listener not registered on the happy path and logs an error-level stack trace per discovered instance. This is noisy and misleading — it looks like a real failure but isn't.

The call itself is still required: per the platform documentation, devices with T SDK extension < 22 do not auto-unregister, so skipping the call would leak listeners on those devices. Instead, catch IllegalArgumentException specifically and log it at warning level without a stack trace, since on modern devices this branch is the expected outcome rather than an error. Other exceptions still log at error level with a stack.

Checklist

• New or updated tests have been added to cover the changes following the testing guidelines.
• The code follows the project's code style and best_practices.
• The changes have been thoroughly tested, and edge cases have been considered.
• Changes are backward compatible whenever feasible. Any breaking changes are documented in the changelog for users and/or in the code for developers depending on the relevance.

Screenshots

Link to pull request in documentation repositories

User Documentation: home-assistant/companion.home-assistant#

Developer Documentation: home-assistant/developers.home-assistant#

Any other notes

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adjusts NSD service resolution teardown to avoid calling stopServiceResolution after the platform has already auto-unregistered the resolve listener.

Changes:

• Track whether resolution has completed and conditionally call stopServiceResolution only on early cancellation.
• Extend getResolvedListener to accept an onDone callback invoked on both success and failure.

[TimoPtr]

I do remember this log while implementing and decided to keep it. If you look at the code of the API 37

 * <p>If the listener is not already registered, for apps running on devices with T SDK
* extension < 22, this will throw with {@link IllegalArgumentException}.

It's cheap to call this even if it throw but it ensure without adding complex logic that in all supported version we properly unregistered the listener.

I get that the log might be disturbing, we could probably capture the IllegalArgumentException and log it as a warning with a message less alarming.

[agners]

I do remember this log while implementing and decided to keep it. If you look at the code of the API 37

 * <p>If the listener is not already registered, for apps running on devices with T SDK
* extension < 22, this will throw with {@link IllegalArgumentException}.

It's cheap to call this even if it throw but it ensure without adding complex logic that in all supported version we properly unregistered the listener.

Well, "cheap" is probably relative. From what I've learned, exceptions are always computationally expensive. Not sure how bad it is in nowadays VMs, but this StackOverflow thread it was measured 60 times slower than no exception. This is without formatting/logging. But I'd guess just the formatting and printing of the stack trace is probably quite expensive already.

So in performance critical code you usually don't want any exception to be raised in regular execution flow. Now this is not performance critical code, so doesn't really apply here, just saying.

I get that the log might be disturbing, we could probably capture the IllegalArgumentException and log it as a warning with a message less alarming.

Yeah and I think that is in my book more important: A unhandled exception showing in logs in regular operation makes me think this got missed. It also makes the logs harder to read.

I'd prefer capturing it and warn over just let it raise. But I also think the current solution is not overly complicated, it really makes sure we don't knowingly run into the exception and generate computational overhead for the VM 🤷 .

[TimoPtr]

I do remember this log while implementing and decided to keep it. If you look at the code of the API 37

 * <p>If the listener is not already registered, for apps running on devices with T SDK
* extension < 22, this will throw with {@link IllegalArgumentException}.

It's cheap to call this even if it throw but it ensure without adding complex logic that in all supported version we properly unregistered the listener.

Well, "cheap" is probably relative. From what I've learned, exceptions are always computationally expensive. Not sure how bad it is in nowadays VMs, but this StackOverflow thread it was measured 60 times slower than no exception. This is without formatting/logging. But I'd guess just the formatting and printing of the stack trace is probably quite expensive already.

So in performance critical code you usually don't want any exception to be raised in regular execution flow. Now this is not performance critical code, so doesn't really apply here, just saying.

In this context it won't impact the usage of the app, vs a potential listener staying forever in the manager. In a more critical part of the app like startup I would argue that we should avoid triggering a raise of an exception that we can potentially avoid when we can.

Today this would impact some devices not all, after testing on the API 37 where I got the documentation no exception are raised anymore.

I get that the log might be disturbing, we could probably capture the IllegalArgumentException and log it as a warning with a message less alarming.

Yeah and I think that is in my book more important: A unhandled exception showing in logs in regular operation makes me think this got missed. It also makes the logs harder to read.

I'd prefer capturing it and warn over just let it raise. But I also think the current solution is not overly complicated, it really makes sure we don't knowingly run into the exception and generate computational overhead for the VM 🤷 .

The general handling of exception within the app is not good. I'm still trying to shape how I want it to evolve in a way I like.

[agners]

Today this would impact some devices not all, after testing on the API 37 where I got the documentation no exception are raised anymore.

Oh wow, ok that is a good argument to keep the call ofc. Let's catch and warn as agreed in chat.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.