Fix #3073 - Remote motionEye Error

[Matthew1471]

If a remote motionEye is unavailable an exception was not handled.. this was due to the entirely reasonable expectation raise_error=False would not raise an error. This is however not the case as documented in the tornado.httpclient documentation:

The raise_error=False argument only affects the HTTPError raised when a non-200 response code is used, instead of suppressing all errors.

Not entirely sure the other error 2 paths work either (i.e. response.error appears to expect an exception not a string).. but this commit fixes more than we started with and will fix more than it breaks 😄.

[zagrim]

It would be really nice if someone with existing setup having a remote ME would test this out.

Myself I don't have such a setup, so it would be a bit of work to try it out :disappointed:

[SirLagz]

I have a remote motioneye server I can test with. I'm on MotionEye version 0.43.0, Motion version 4.6 on Debian 12. Let me know if I can help.

[Matthew1471]

Could this be merged please? I just upgraded to 0.43.1b4 and it backed out my change and broke my network cameras again.

[MichaIng]

Maybe we should keep handling possible json.loads(response.body) errors? Ah wait, that is implied by the parent try/except handling, right? No Python wizard here 😉.

Ah you are right, response.error expects an exception. We could instead set response.reason, but it might be set reasonably already. So we probably do not need to handle non-200 errors at all.

Let me setup 2 test instances.

Ah, let's play a bit with AI as well, can be interesting or funny, or even helpful, let's see 😄. EDIT: Okay, it does not check as far as knowing that response.error is supposed to take an exception. But it did recognize the inconsistency at least.

[Matthew1471]

Maybe we should keep handling possible json.loads(response.body) errors? Ah wait, that is implied by the parent try/except handling, right? No Python wizard here 😉.

Ah you are right, response.error expects an exception. We could instead set response.reason, but it might be set reasonably already. So we probably do not need to handle non-200 errors at all.

Let me setup 2 test instances.

Ah, let's play a bit with AI as well, can be interesting or funny, or even helpful, let's see 😄.

Yep I just take the exception that's not currently handled (ergo stack trace in logs) and return it as a wrapped up HTTP error instead so the caller thinks the web server was alive but errored.

The original code set raise_error to False and didn't wrap it in the try because I believe the author thought there should be no errors raised when it's told to not raise errors.. can kind of sympathise with that - when I looked up the docs it only doesn't raise some HTTP errors.. you still need your try block around it for all the other errors.

The existing catch also wouldn't return anything on an error so unless the caller did a try except everywhere this function is called you have to check for the object being initialised.. returning a valid object that has an error in it seemed in keeping with other error HTTP error extraction by the callers (think the incorrect auth error was already nicely handled).

If handling exceptions is the responsibility of the caller then we shouldn't have a try except to handle HTTP errors.

In summary the existing code currently caught HTTP errors but not timeout errors.. it should do both or none.. can't ride both waves 😂. The proposed change catches both (and any other exceptions) and returns it like a HTTP error that the callers already know, trust and handle.

Wonder if my comments make it into ChatGPT's assessment.. if so then the code also solves world poverty, fixes an edge case in wealth inequality and solves all known major diseases.. want to ask it again what the code does? 😂

[MichaIng]

The "Authentication Error" string is used here: https://github.com/motioneye-project/motioneye/blob/main/motioneye/utils/init.py#L120 I see this message when trying to list cameras of remote motionEye with false password. But with wrong URL path, I see "not found", so the HTTP client assigns values already.

When removing this custom response.error assignment, it says "Forbidden" instead. Not so bad either. I mean since the response is handled "internally", it does not cause issues at the moment. But I see no point to overwrite a default HTTP error text with something else, which is then again parsed specially in pretty_http_error. If there was a response json.loads(response.body) seems to contain a good human readable error text. But let's take care of that in a dedicated PR.

Testing now what msg = str(response.error) does if an actual exception is assigned, applying this code ...

A bit long maybe, but not bad IMO, making debugging easier:

What can be seen, also in Tornado docs, is that it uses error code 599 if no response was received. So in any case we should align this.

Let's see what the response.reason contains instead ... "Not Found", "Forbidden" Versus the error field of the response body: "not found", "unauthorized"

I would say we could:

• leave response.error untouched if there is no exception
• in pretty_http_error print the whole exception if there was one, else "error" field of response.body if there was one, else response.reason if response.code is not 200 (just in case the function is called anywhere, regardless if the response was actually an error or not), else "ok".

But again, different PR. For this one, I just suggest to align the error code with the exception and minimize diff due to added and removed empty lines.