ldb/lua-telegram-bot

Wrong status code crashes app

tapir opened this issue · 5 comments

tapir commented

Today api.telegram.org gives "502 bad gateway error" or "504 gateway timeout" error sometimes. Somehow response.status is 1 so the error page is sent to JSON decoding and everything comes down burning. I'm not sure if it's a luasec problem or it's used wrongly in this library but I've managed to solve it buy changing this line

success = success or false

to this line

success = tonumber(code) < 400

and all the

response.success == 1

to

response.success

PS: I didn't create a pull request for this because I don't know if this is luasec's fault or not.
Edit: Fixed the solution

ldb commented

Hello,

you are right, 502 bad gateway error or 504 gateway timeout are not caught.
The way it works is that
response.success returns 1 if the actual request was successful, and nil if not.
This does not include the actual HTTP response code, which is returned in response.code.

The reason I don't check for the actual response code, is because other codes such as
401 Unauthorized still return valid JSON, which can (and should) be parsed, to make debugging of API calls easier.

The problem is that the Server does not return (valid) JSON on the errors above.

So, instead of mixing status and response variables, which might leave errors such as 401 flagged as a false positive, a better approach would be to catch errors produced while decoding JSON.

I will include functions to handle bad JSON errors.
Also, I may include some sort of logging.

If you have records of the errors you received, you can post them here, so I can have a look.

tapir commented

I'm not very informed about HTTP status codes but shouldn't, at least, every 5xx error cause a "status.success = false". There is practically no difference between an unsuccessful HTTP request and a server error, as far as a telegram bot is concerned.

Also how does one get 401 from api.telegram.org?

ldb commented

If you want to handle it that way, you can.
However other people might want to react differently to those errors, and I would feel bad for taking this opportunity from them, since I want to leave as many possibilities open as possible.

Please check out the latest commit in the dev branch (@a512957).
It adds JSON Error handlers which do not assert on en-/decoding, thus should no longer crash the bot.

You can get error 401 Unauthorized from the API by providing a wrong token.

tapir commented

Fair enough. But then below statement and code are in contradiction.

response.success returns 1 if the actual request was successful, and nil if not.
This does not include the actual HTTP response code, which is returned in response.code.

  if (response.success == 1) then
    return JSON:decode(response.body)
  else
    return nil, response.code
  end

If luasec status is not related to the HTTP status code why are we returning it when there is an unrelated error?

ldb commented

Good observation, this indeed does not make much sense.
I will put in a more descriptive error message later.

Also, upon error, response.success does not become nil, as previously stated, but "false".

I presume the issue you had should be closed by now, please try it out.