Suggestion: More exception subclasses?

[caffodian]

There are a lot of places in the SDK where common exception classes are re-used to handle different HTTP response codes from the REST API.

For example:

https://github.com/opentok/Opentok-Python-SDK/blob/master/opentok/opentok.py#L522

This is okay for catastrophic errors, where we want it to fail loudly, but if we want to handle these exceptions gracefully, it's required that we parse the exception string for details. This can be a bit frustrating and error prone.

In the signalling error case above, there are many cases where we might want to just ignore the exception if we tried to send a signal to a now-gone connection. We probably still want it to fail loudly if we just have insufficient credential levels.

Thanks!

[jenstroeger]

This may be somewhat unrelated, but since we’re talking about exception handling here… Today I encountered the following exception:

 ...
 File "/…/lib/python3.7/site-packages/opentok/opentok.py", line 592, in force_disconnect
   endpoint, headers=self.json_headers(), proxies=self.proxies, timeout=self.timeout
 File "/…/lib/python3.7/site-packages/requests/api.py", line 158, in delete
   return request('delete', url, **kwargs)
 File "/…/lib/python3.7/site-packages/requests/api.py", line 60, in request
   return session.request(method=method, url=url, **kwargs)
 File "/…/lib/python3.7/site-packages/requests/sessions.py", line 533, in request
   resp = self.send(prep, **send_kwargs)
 File "/…/lib/python3.7/site-packages/requests/sessions.py", line 646, in send
   r = adapter.send(request, **kwargs)
 File "/…/lib/python3.7/site-packages/requests/adapters.py", line 514, in send
   raise SSLError(e, request=request)
SSLError: HTTPSConnectionPool(host='api.opentok.com', port=443): Max retries exceeded with url: /v2/project/…/session/…/connection/24…ca (Caused by SSLError(SSLEOFError(8, 'EOF occurred in violation of protocol (_ssl.c:1076)')))

In force_disconnect(), like in any other endpoint wrapper, the same exception ForceDisconnectError() is raised for different errors; see @caffodian’s comment.

However, exceptions raised by the underlying requests layer are not being caught and propagate further up the call stack. That probably is intentional. Considering the tokbox.RequestError() though, I am tempted to argue that exceptions raised by the requests layer should be caught and re-raised as RequestError? And because RequestError derives from OpenTokException, client code like mine wouldn’t be surprised by exceptions coming from layers which OpenTok wraps in the first place.

For now, I will add a

try:
    # Call some OpenTok function.
except OpenTokException:
    …
except requests.exceptions.RequestException:
    …

to all of my OpenTok calls.

[caffodian]

That's slightly horrifying! I hadn't run into that in production yet, but catching and reraising does seem a bit reasonable (and we don't have to know whether opentok uses requests under the hood, or not.

[superdiana]

Hello! I'll be taking over the sdk and will be working on these issues. I appreciate your inputs and feedback!