karbassi / netatmo

A node.js module to hook into the netatmo API.
MIT License
58 stars 46 forks source link

parse body only when content-type is 'application/json' #15

Closed janhuddel closed 8 years ago

janhuddel commented 8 years ago

I have another fix for you. Problem: under certain circumstances the authenticate_refresh-function received an non-json-body. In that case the JSON.parse in handleRequestError throws an exception which meant that my program (node-red) was terminated. Before parsing the body I check the content-type of the response-header. JSON.parse will only called when content-type is 'application/json'. I hope that will fix my problem (I can't test it really good because this problem occurs very rarely).

danilowanner commented 8 years ago

@janhuddel thanks for that. Our application also experienced this bug occasionally. I did not do anything about it since our docker container automatically restarts when that happens. Maybe someone can confirm your fix or you can post an update when you did some long-term testing.

karbassi commented 8 years ago

@janhuddel Good catch.

What does the response say when this error is thrown? Is there any other times where a non-json-body is ever thrown?

janhuddel commented 8 years ago

@karbassi I don't know the body of the response. The only message I have seen is this one: SyntaxError: Unexpected token < at Object.parse (native)

But there is another point in this context: if the refresh-process fails at this point the api is unusable. if I have understood correctly, you are updating the token only via the timeout function. There is no retry. Would not it be safer if you check the token before each request?

karbassi commented 8 years ago

@janhuddel checking the token after (or before) each request would add additional process time for the request which isn't needed.

If you print out the body after line 36, what is the message returned? I'm assuming a string with HTML in it, since the error you noted above has a < in it.

janhuddel commented 8 years ago

Hm, checking the expires_in before each call isn't so expensive I think... The refresh-token-process should only initiated if the token has expired.

Yes, the body seems to be html. But I can't reproduce the error :( I've added a console.log(body) in the method. Next time when I get this error I will post the message here.

karbassi commented 8 years ago

@janhuddel if you want to create another PR for checking expires_in, that would be great.

As for the error, let's see what comes back and update the code accordingly.

janhuddel commented 8 years ago

Hi.

the fix for checking expires_in before each request is a little bit more complex than I expected. I don't know if I have enough time to create another PR for this.

Back to topic: I will have have a look at the console the next days. If I get more info about the problem while token-refresh I will inform you.

janhuddel commented 8 years ago

Hi,

today I caught folling error in handleRequestError: `2016-02-19T07:09:48.715Z [handleRequestError]: body=

502 Bad Gateway

502 Bad Gateway


nginx

, response.statusCode=502, content-type=text/html `

I also got errors like theese (ok, because valid json): 2016-02-19T13:09:14.597Z [handleRequestError]: body={"error":{"code":500,"message":"Internal Server Error"}}, response.statusCode=500, content-type=application/json; charset=utf-8

And this is the 3rd form of errors here: 2016-02-19T09:40:12.079Z [handleRequestError]: body=undefined

The response-object in the 3rd error-case is undefined (like the body).

It seems the netatmo-server has some outtakes sometimes...

Greetings Jan

karbassi commented 8 years ago

@janhuddel Is this still an issue?

janhuddel commented 8 years ago

Yes. I can see many non-json responses from netatmo in the log. It would be great if you merge this PR into your master.