Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cryptic error message when HTTP 504 error code is received #15

Open
e13h opened this issue Jul 1, 2021 · 3 comments
Open

Cryptic error message when HTTP 504 error code is received #15

e13h opened this issue Jul 1, 2021 · 3 comments

Comments

@e13h
Copy link

e13h commented Jul 1, 2021

The issue

When a response with an error status code other than 429 is received (like 504), a cryptic error message about parsing the results is printed to the console.

Details

Currently when I use the following code to make a couple hundred requests, I get the following error after about 1 minute of spinning:

pk_api.lookup_placekeys(some_queries)
JSONDecodeError: Expecting value: line 1 column 1 (char 0)

With the following stack trace:

----> pk_api.lookup_placekeys(query)

/.../python3.7/site-packages/placekey/api.py in lookup_placekeys(self, places, strict_address_match, strict_name_match, batch_size, verbose)
    200                     places[i:max_batch_idx],
    201                     strict_address_match=strict_address_match,
--> 202                     strict_name_match=strict_name_match
    203                 )
    204             except RateLimitException:

/.../python3.7/site-packages/placekey/api.py in lookup_batch(self, places, strict_address_match, strict_name_match)
    270                 break
    271 
--> 272         return json.loads(result.text)
    273 
    274     def _validate_query(self, query_dict):

/.../python3.7/json/__init__.py in loads(s, encoding, cls, object_hook, parse_float, parse_int, parse_constant, object_pairs_hook, **kw)
    346             parse_int is None and parse_float is None and
    347             parse_constant is None and object_pairs_hook is None and not kw):
--> 348         return _default_decoder.decode(s)
    349     if cls is None:
    350         cls = JSONDecoder

/.../python3.7/json/decoder.py in decode(self, s, _w)
    335 
    336         """
--> 337         obj, end = self.raw_decode(s, idx=_w(s, 0).end())
    338         end = _w(s, end).end()
    339         if end != len(s):

/.../python3.7/json/decoder.py in raw_decode(self, s, idx)
    353             obj, end = self.scan_once(s, idx)
    354         except StopIteration as err:
--> 355             raise JSONDecodeError("Expecting value", s, err.value) from None
    356         return obj, end

This was quite puzzling, especially since the exact same code worked just 30 minutes prior (with the exact same queries, too).

It seemed obvious that whatever lookup_batch received was not status code 429 (thus broke the while loop), but did not contain any JSON in the response text

placekey-py/placekey/api.py

Lines 266 to 272 in 6d13e53

# Make request, and retry if there is a server-side rate limit error
while True:
result = self.make_bulk_request(batch_payload)
if result.status_code != 429:
break
return json.loads(result.text)

I called make_bulk_request directly to see what the real result was and got more helpful output.

pk_api.make_bulk_request({"queries": query[:100]})
<Response [504]>

Suggested solution

Perhaps lookup_batch could raise an error if a non-200 HTTP status is received? Or perhaps just certain 500 codes? Or more of a catch-all error if the result text isn't JSON?

# Make request, and retry if there is a server-side rate limit error
while True: 
    result = self.make_bulk_request(batch_payload)
    if result.status_code != 429: 
        break

if not result.text:
    raise RuntimeError(f"Error: Status code {result.status_code}")

return json.loads(result.text) 
@bnaul
Copy link

bnaul commented Apr 26, 2024

It appears that this is still an issue, in my case I'm seeing 502s not 504 but they also manifest as the same JSONDecodeError. Even just doing request.raise_for_status() before returning seems like an improvement here. @jarredSafegraph any thoughts?

@jarredSafegraph
Copy link
Collaborator

@bnaul Slight degradation in service, recovering now and should be back to normal shortly. Sorry for any issue.

@bnaul
Copy link

bnaul commented Apr 26, 2024

Thanks @jarredSafegraph, I would still say though that the error handling could be better for such cases (unless you think this will be the last ever 50* error a user ever sees 😅 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants