call_service() swallows exceptions

[Mithras]

        result = await self.call_service("this/fails")
        self.log(f"result={result}")

result=None

Why do you swallow exceptions and return None?

[Odianosen25]

@Mithras,

There is no exception to that, but there should be a warning in your logs stating that such a service doesn’t exists.

Regards

[Mithras]

Appdaemon 3 was throwing exception. Appdaemon 4 is swallowing it - https://github.com/home-assistant/appdaemon/blob/dev/appdaemon/services.py#L113. Swallowing exceptions is the worst anti pattern ever.

[Odianosen25]

Nope this is the None that is returned in your example https://github.com/home-assistant/appdaemon/blob/dev/appdaemon/services.py#L67. If there is actually an error in the call_service function, then the error is sent to the error log https://github.com/home-assistant/appdaemon/blob/dev/appdaemon/services.py#L111.

[Mithras]

Ignore the example. Ok, it might be a wrong line but in log I'm seeing

2020-03-10 09:11:46.964798 WARNING HASS: Error calling Home Assistant service default/camera/record 2020-03-10 09:11:46.965925 WARNING HASS: Code: 500, error: 500 Internal Server Error

And Appdaemon 3 was for sure throwing an exception for this. I expect this exception (happens when camera is already recording) and was catching it before. Now it just returns None.

[Mithras]

Anyway, there are 4 "return None" there. How do I even tell what's wrong? Why doesn't it just throw exceptions with adequate messages? Logging doesn't replace exceptions. Bad design.

[Odianosen25]

Its not an issue to raise an exception, will let @acockburn know abt this. We could just raise a value error or something, since a value is missing

[Odianosen25]

@Mithras The example you gave, actually is not from AD but from HA. I was writing based on the earlier example, but from what you just posted, it means you send a wrong service call to HA or something, and it gave that.

[Mithras]

It's not wrong it's just that if you call "camera/record" when it's already recording, it returns 500 which I don't want Appdaemon to swallow. Anyway, if you are interested in what I'm doing, you can look at the code here: https://github.com/Mithras/ha/blob/tmp/appdaemon/apps/camera_alarm/camera_alarm.py

[acockburn]

Happy to fix the issue - less happy about "anti-patterns" and accusations of bad design. Please keep this respectful, this is our hobby and we are giving up our spare time to help you.

[Mithras]

Happy to fix the issue - less happy about "anti-patterns" and accusations of bad design. Please keep this respectful, this is our hobby and we are giving up our spare time to help you.

I'm sorry. You guys are doing an amazing job here and I truly appreciate it. It doesn't make swallowing/not using exceptions any less anti pattern though. I'm not good at phrasing things in a nice way but I'm just trying to describe the issue. I'm not trying to blame anybody or diminish the amazing work you are doing.

[acockburn]

Thanks - I appreciate that. We'll look into restoring the previous behavior.

[Mithras]

I might also contribute some in future. As of today I'm pretty new to python and not feel comfortable contributing yet.

[acockburn]

We'll be happy to see that when you are ready :)

[ReneTode]

Appdaemon 3 was throwing exception. Appdaemon 4 is swallowing it

in AD 3 call service returned None. and no exceptions. see the docs from AD 3: https://appdaemon.readthedocs.io/en/3.0.5/HASS_API_REFERENCE.html#services

if the service in HA didnt exist or did go wrong you would have had an error in your logs, just like it does now.

[dlashua]

@Mithras I agree with you that the Exception (specifically a 500 or 404 on service call) should be raised to the user for them to do with it as they please.

While swallowing all exceptions is an "anti-pattern", in many places in AppDaemon it is needed. This is because we do not want an unhandled exception in one app to cause all apps to fail. Our users often write their apps in a LIVE system and we don't want one app to negatively affect any others. However, there are some cases where we are swallowing the exception too low in the chain and it would be more beneficial to catch it at a higher level to give the user the option to catch it and do something with it. We have this on our list of things to review.