rbw / pysnow

ServiceNow API Client Library
MIT License
204 stars 90 forks source link

Handling HTTP response 202 #122

Closed maxdevyatov closed 5 years ago

maxdevyatov commented 5 years ago

ServiceNow can reply with empty response with HTTP 202 status code. Since the response is empty call to self._response.json() in Response._get_buffered_response raises an exception.

rbw commented 5 years ago

Thanks for reporting. I'll look into this shortly.

maxdevyatov commented 5 years ago

Sorry it took too long to get more details. I get this error when I do more than ten parallel get requests with threads.

_response.status_code: 202
_response.headers: {'Content-Length': '0', 'Date': 'Thu, 03 Oct 2019 19:02:06 GMT', 'Server': 'ServiceNow', 'Strict-Transport-Security': 'max-age=63072000; includeSubDomains'}
_response.content: b''
Traceback (most recent call last):
<......>
  File "/usr/local/lib/python3.7/site-packages/pysnow/response.py", line 208, in one
    result, count = self._get_buffered_response()
  File "/usr/local/lib/python3.7/site-packages/pysnow/response.py", line 138, in _get_buffered_response
    result = self._response.json().get('result', None)
  File "/usr/local/lib/python3.7/site-packages/requests/models.py", line 897, in json
    return complexjson.loads(self.text, **kwargs)
  File "/usr/local/Cellar/python/3.7.3/Frameworks/Python.framework/Versions/3.7/lib/python3.7/json/__init__.py", line 348, in loads
    return _default_decoder.decode(s)
  File "/usr/local/Cellar/python/3.7.3/Frameworks/Python.framework/Versions/3.7/lib/python3.7/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/local/Cellar/python/3.7.3/Frameworks/Python.framework/Versions/3.7/lib/python3.7/json/decoder.py", line 355, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)
rbw commented 5 years ago

Interesting - I'm guessing these are PUT, PATCH or POST requests that are being throttled/queued?

I'll look into this during the week. Again, thanks for reporting.

maxdevyatov commented 5 years ago

This was a GET request. There might've been other GET and PUT requests executed in parallel. response = self.myresource.get(query={'u_id': my_id}, fields=['sys_id'])

rbw commented 5 years ago

Should be fixed in #123 , can you verify @maxdevyatov ?

maxdevyatov commented 5 years ago

I am not sure if this is a proper way of handling it.

An empty GET response currently means that there is no requested entry in the table. With this fix you can get an empty response and it can also mean that there was a server error. Unapparent way to verify this will be to check ._response.status_code.

Also, Code 202 can be seen on PUT requests with no real updates in the table after that.
Found some info here https://community.servicenow.com/community?id=community_blog&sys_id=ee3eee6ddbd0dbc01dcaf3231f961949

I guess it would be better if it could raise an exception like other error codes that are caught by response.raise_for_status(). Then it will be easy to handle in the app.

maxdevyatov commented 5 years ago

It raises JSONDecodeError now but it took some time to understand what it really is. It would be just nice to have more meaningful exception raised by pysnow. Silently hiding it would be undesirable.

rbw commented 5 years ago

Silently hiding it would be undesirable.

The intention was not to hide it, but rather to handle 202-responses properly (these request should never return any JSON, so let's not even attempt decoding).

It would be just nice to have more meaningful exception raised by pysnow.

Yes, I agree -- raising an exception for status 202 in these cases would be ideal. However, I need to do some investigating and testing of this; If a 202 is actually returned both when queued/throttled and on successful updates (for instance), I'll have to come up with a better solution.

maxdevyatov commented 5 years ago

Tested #123 and Response.one() method returns an empty dict where I beleive it is better to raise an exception.

response = self.resource.get(query={'u_id': id}, fields=['sys_id'])
response._response.headers: {'Content-Length': '0', 'Date': 'Wed, 23 Oct 2019 17:13:42 GMT', 'Server': 'ServiceNow', 'Strict-Transport-Security': 'max-age=63072000; includeSubDomains'}
response._response.content: b''
response._response.status_code: 200
response.one(): {}
response.one()['sys_id'] -> KeyError: 'sys_id'
rbw commented 5 years ago

Status 202 for GET requests doesn't make sense in a RESTful HTTP API - unless the creation of the actual object was postponed due to a large amount of requests. Could this be the case?

Also, this code isn't mentioned in the ServiceNow docs -- are you sure the response actually comes from ServiceNow and not a proxy?

Furthermore, if this is actually returned instead of 429 or 503 for requests due to rate limiting, then it's probably a misdesign of the ServiceNow API (but I doubt that's the case).

I can create a custom exception for this, no biggie, but first I want to know what's going on.

maxdevyatov commented 5 years ago

I do a large amount of GET and PUT requests in parallel using pysnow with 10-15 threads. Everything works well single-threaded. There is no proxy on my side and I use SN developer instance in the SN cloud. Not very familiar with the architecture but I guess those are managed by SN. Also, note 'Server': 'ServiceNow' line in the response header.

I agree this status code doesn't make much sense and I wasn't able to find it in the documentation. But here https://community.servicenow.com/community?id=community_blog&sys_id=ee3eee6ddbd0dbc01dcaf3231f961949 is an article from SN support engineer where he explains how it works.

I do not usually check response codes and noticed the problem only for GET requests after I got JSONDecodeError exception when I tried to read the responses. I will try to add a manual check for non-200 codes after every request and see if is returned for PUT requests as well. Also, I just noticed that my latest debug output has status code 200. Probably I posted a wrong output. I will doublecheck.

rbw commented 5 years ago

I am able to reproduce this with GET requests, but I don't think the article you linked explains this behavior.

Here is what rfc7231 has to say about 202:

   The 202 (Accepted) status code indicates that the request has been
   accepted for processing, but the processing has not been completed.
   The request might or might not eventually be acted upon, as it might
   be disallowed when processing actually takes place.  There is no
   facility in HTTP for re-sending a status code from an asynchronous
   operation.

   The 202 response is intentionally noncommittal.  Its purpose is to
   allow a server to accept a request for some other process (perhaps a
   batch-oriented process that is only run once per day) without
   requiring that the user agent's connection to the server persist
   until the process is completed.  The representation sent with this
   response ought to describe the request's current status and point to
   (or embed) a status monitor that can provide the user with an
   estimate of when the request will be fulfilled.

I'm beginning to believe this is a ServiceNow API design flaw. We're clearly hitting an error here; no data back for a GET request, and this is being treated as a success. Instead, code 429 would be more appropriate.

I'll create an exception for this, that's not raised by default (as the opposite might break existing implementations).

Sounds all right?

maxdevyatov commented 5 years ago

It will work for me if there is a way to enable this exception.

rbw commented 5 years ago

Hello,

Sorry for the delay (again).

Making this optional added too much complexity; EmptyContent is now always raised for GET: "202 Accepted".

@maxdevyatov Can you verify the PR?

rbw commented 5 years ago

Fixed in 0.7.11