Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[knx] Handle exceptions during initial read #12520

Merged
merged 1 commit into from
Apr 8, 2022

Conversation

holgerfriedrich
Copy link
Member

fixes #7239

Signed-off-by: Holger Friedrich [email protected]

@holgerfriedrich
Copy link
Member Author

factored out from #12434

@holgerfriedrich holgerfriedrich added enhancement An enhancement or new feature for an existing add-on bug An unexpected problem or unintended behavior of an add-on and removed enhancement An enhancement or new feature for an existing add-on labels Mar 25, 2022
@lolodomo
Copy link
Contributor

@jlaur : please take the lead to review this PR, your comments are more detailed than mine.

@holgerfriedrich
Copy link
Member Author

@jlaur I had a closer look into the implementation of the Calimero library. The function called in the try block may throw more exceptions than declared,,,,

ProcessCommunicatior.read(...) declares KNXException, InterruptedException

but it may throw other exceptions as well:

readFromGroup declares InterruptedException and subclasses of KNXException -> may throw
IllegalStateException extends RuntimeException extends Exception if detached
InterruptedException extends Exception in case secure GO is interrupted
CancellationException extends IllegalStateException extends RuntimeException extends Exception in case secure GO is interrupted
KnxTimeoutException extends KnxException in case secure GO times out

extractGroupASDU -> may throw KnxIllegalArgumentException extends KnxRuntimeException extends RuntimeException extends Exception in case APDU length does not match configured data type
setdata -> may throw KnxIllegalArgumentException extends KnxRuntimeException extends RuntimeException extends Exception

createTranslator -> may throw
[IndexOutOfBoundsException extends RuntimeException extends Exception]
NumberFormatException extends IllegalArgumentException extends RuntimeException extends Exception DPT type cannot be parsed
KnxException no translator, other exceptions re-thrown as KnxException

DataUnitBuilder.extractASDU -> may throw KNXIllegalArgumentException extends RuntimeException extends Exception wrong APDU data

So I now changed the code do catch any exception.

The rationale behind it is as follows: any exception will end all scheduled reads, as the scheduled job ends. Catching exceptions and printing error messages is the approach, as it only skips the one faulty DP and continues for all others.
All exceptions which could be resolved with a retry should be handled in the first catch(KNXException) clause. Interruption is handled separately, as no log with stack trace is needed. All other stuff is printed as a warning, as I do not want to hide any problem.

@jlaur
Copy link
Contributor

jlaur commented Apr 8, 2022

So I now changed the code do catch any exception. The rationale behind it is as follows: any exception will end all scheduled reads, as the scheduled job ends.
[....]

Thanks for following up on this. I think it's fine, since you are calling an external library you have no control over. So you need to encapsulate the risk of anything than can happen outside your boundary.

Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix.

@jlaur jlaur merged commit 7e35341 into openhab:main Apr 8, 2022
@jlaur jlaur added this to the 3.3 milestone Apr 8, 2022
@jlaur jlaur changed the title [knx] handle exceptions during initial read [knx] Handle exceptions during initial read Apr 8, 2022
@holgerfriedrich holgerfriedrich deleted the 7239-pr-knx-startup branch April 8, 2022 21:21
NickWaterton pushed a commit to NickWaterton/openhab-addons that referenced this pull request Apr 27, 2022
fixes openhab#7239

Signed-off-by: Holger Friedrich <[email protected]>
Signed-off-by: Nick Waterton <[email protected]>
andan67 pushed a commit to andan67/openhab-addons that referenced this pull request Nov 6, 2022
andrasU pushed a commit to andrasU/openhab-addons that referenced this pull request Nov 12, 2022
fixes openhab#7239

Signed-off-by: Holger Friedrich <[email protected]>
Signed-off-by: Andras Uhrin <[email protected]>
psmedley pushed a commit to psmedley/openhab-addons that referenced this pull request Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[knx] initial addresses read at startup breaks after exception
3 participants