[smhi] Fix exception in aggregation function when daily forecast is empty

[pacive]

Also catch any Exception to prevent thread from crashing

Fixes #10850

Signed-off-by: Anders Alfredsson [email protected]

[fwolter]

Catching a division by zero exception, raised in your own code isn't a pretty good solution. Better check if the divisor is zero beforehand.

[pacive]

Better check if the divisor is zero beforehand.

That's what I did, the method returns Optional.empty() if the expression used as divisor returns zero.

openhab-addons/bundles/org.openhab.binding.smhi/src/main/java/org/openhab/binding/smhi/internal/ForecastAggregator.java

Lines 41 to 43 in b4a61f5

	if (dayForecasts.isEmpty()) {
	return Optional.empty();
	}

The catch-all is just so the binding won't crash if there are other exceptions thrown that I might have missed.

[fwolter]

Ok, I missed that. Catching unchecked exceptions in binding's scheduler threads is not necessary as the framework will catch and log them.

[pacive]

Yes, but if the exception is caught by the scheduler it stops scheduling new tasks, so the binding stops updating.

[pacive]

I know that it's probably bad practice to catch Exception, but is there a better way to prevent overlooked bugs from causing the binding to stop working?

[fwolter]

Are you sure the periodic task is cancelled completely? Actually, we came to the conclusion in openhab/openhab-core#2392 that this is covered for bindings.

[pacive]

The user who reported the issue had the binding stop updating, and I had a different issue earlier (which is already patched) that also caused the same symptom. The discussion you linked only seems to suggest that the exceptions are logged, not that the execution of the scheduled tasks continue?

Should this be filed as an issue in oh core? I can see cases where you don't want the task to continue be scheduled, e.g. to avoid tasks that consistently throws exception to continue to run, possibly hogging resources (which I admit would also be the case if everyone catches Exception in all tasks)

[fwolter]

You're right. The exception is logged and the periodic task is terminated. I think is is okay behavior. It's ok to let the binding crash (loudly) if it encounters a bug that needs to be fixed.

I'd remove the generic try-catch if it was my code as I think bugs should be fixed and not handled. I leave it up to you if you keep it or not. But if you keep it, you should make the log level error.

[pacive]

I'd remove the generic try-catch if it was my code as I think bugs should be fixed and not handled. I leave it up to you if you keep it or not. But if you keep it, you should make the log level error.

I totally agree, but I also want to spare users from problems caused by my mistakes. I narrowed down the try-catch to RuntimeException and changed the logging to warn, with a suggestion to report the issue so it can be fixed properly. While working on fixing this one better I also made some changes which I believe should make it a bit more robust.

[fwolter]

LGTM