zulip: Reraise exceptions in do_api_query. #735

eeshangarg · 2021-11-07T19:49:28Z

There are cases where the call to get_server_settings returns a JSON response with a connection error. In the case of such an unsuccessful response, we should raise an appropriate exception instead of parsing the response as though it was successful.

@timabbott Could you please take a look at this? Not sure if this is the right fix. Also, wondering if I should put a test for this somewhere in zulip_botserver?

timabbott · 2021-11-13T00:58:47Z

This looks correct to me. @andersk FYI.

For testing, I would prefer a test for the main zulip module, since the code is for that module. I'm not sure we are setup to make it easy to write such a test, but at the very least I'd like to see the output from trying to connect to a non Zulip server with this.

andersk · 2021-11-13T01:51:17Z

zulip/zulip/__init__.py

        server_settings = self.get_server_settings()
+
+        if server_settings["result"] != "success":
+            raise ZulipError(server_settings["msg"])


It seems to me that get_server_settings() itself should do this check and raise an exception—this is a case of #672.

The work to resolve #672 seems outside of the scope of this PR since this isn't really an endpoint-specific error, but a generic one that we return ourselves. If you look at this line, would it be better to treat this timeout exception as an UnrecoverableNetworkError and raise it there in do_api_query (just like we do for SSL errors)? That way, we can keep the scope of this PR relatively small and think about #672 separately?

Huh that points out an error in my own phrasing of the commit message, this isn't really specific to get_server_settings, now that I think about it.

The point here is not specific to timeout exceptions: The point is that any error that results in the server_settings request failing should result in an exception being raised. It would probably be better for the "original" exception to be reraised.

I agree this is a special case of #672, though I think the right fix for that probably involves making do_api_query do the right thing with exceptions. Do we think this is a reasonable tactical fix and we should just extend #672 with a note that we should remember to remove this when we fix it?

@timabbott I just pushed some changes and they are what would represent a tactical fix we could possibly merge. Please let me know what you think! Thanks! :)

timabbott · 2021-11-19T06:28:23Z

zulip/zulip/__init__.py

+                            "result": "connection-error",
+                        }
+                    else:
+                        raise UnrecoverableNetworkError(msg)


Hmm, what does this look like in terms of output? I'm not sure that we don't want to just reraise e.

It’s definitely not ideal to raise an exception whose message is the stringified traceback of another exception, unless you’re getting paid per line of traceback.

>>> c.get_server_settings() Traceback (most recent call last): File "/home/anders/zulip/python-zulip-api/.direnv/python-3.10.0/lib/python3.10/site-packages/urllib3/connectionpool.py", line 445, in _make_request six.raise_from(e, None) File "<string>", line 3, in raise_from File "/home/anders/zulip/python-zulip-api/.direnv/python-3.10.0/lib/python3.10/site-packages/urllib3/connectionpool.py", line 440, in _make_request httplib_response = conn.getresponse() File "/nix/store/33jhvwkqm1327ypyhzbvl1l5d6ch62jn-python3-3.10.0/lib/python3.10/http/client.py", line 1368, in getresponse response.begin() File "/nix/store/33jhvwkqm1327ypyhzbvl1l5d6ch62jn-python3-3.10.0/lib/python3.10/http/client.py", line 317, in begin version, status, reason = self._read_status() File "/nix/store/33jhvwkqm1327ypyhzbvl1l5d6ch62jn-python3-3.10.0/lib/python3.10/http/client.py", line 278, in _read_status line = str(self.fp.readline(_MAXLINE + 1), "iso-8859-1") File "/nix/store/33jhvwkqm1327ypyhzbvl1l5d6ch62jn-python3-3.10.0/lib/python3.10/socket.py", line 705, in readinto return self._sock.recv_into(b) File "/nix/store/33jhvwkqm1327ypyhzbvl1l5d6ch62jn-python3-3.10.0/lib/python3.10/ssl.py", line 1273, in recv_into return self.read(nbytes, buffer) File "/nix/store/33jhvwkqm1327ypyhzbvl1l5d6ch62jn-python3-3.10.0/lib/python3.10/ssl.py", line 1129, in read return self._sslobj.read(len, buffer) TimeoutError: The read operation timed out During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/home/anders/zulip/python-zulip-api/.direnv/python-3.10.0/lib/python3.10/site-packages/requests/adapters.py", line 439, in send resp = conn.urlopen( File "/home/anders/zulip/python-zulip-api/.direnv/python-3.10.0/lib/python3.10/site-packages/urllib3/connectionpool.py", line 755, in urlopen retries = retries.increment( File "/home/anders/zulip/python-zulip-api/.direnv/python-3.10.0/lib/python3.10/site-packages/urllib3/util/retry.py", line 532, in increment raise six.reraise(type(error), error, _stacktrace) File "/home/anders/zulip/python-zulip-api/.direnv/python-3.10.0/lib/python3.10/site-packages/urllib3/packages/six.py", line 770, in reraise raise value File "/home/anders/zulip/python-zulip-api/.direnv/python-3.10.0/lib/python3.10/site-packages/urllib3/connectionpool.py", line 699, in urlopen httplib_response = self._make_request( File "/home/anders/zulip/python-zulip-api/.direnv/python-3.10.0/lib/python3.10/site-packages/urllib3/connectionpool.py", line 447, in _make_request self._raise_timeout(err=e, url=url, timeout_value=read_timeout) File "/home/anders/zulip/python-zulip-api/.direnv/python-3.10.0/lib/python3.10/site-packages/urllib3/connectionpool.py", line 336, in _raise_timeout raise ReadTimeoutError( urllib3.exceptions.ReadTimeoutError: HTTPSConnectionPool(host='anders-test.zulipchat.com', port=443): Read timed out. (read timeout=15.0) During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/home/anders/zulip/python-zulip-api/zulip/zulip/__init__.py", line 629, in do_api_query res = self.session.request( File "/home/anders/zulip/python-zulip-api/.direnv/python-3.10.0/lib/python3.10/site-packages/requests/sessions.py", line 542, in request resp = self.send(prep, **send_kwargs) File "/home/anders/zulip/python-zulip-api/.direnv/python-3.10.0/lib/python3.10/site-packages/requests/sessions.py", line 655, in send r = adapter.send(request, **kwargs) File "/home/anders/zulip/python-zulip-api/.direnv/python-3.10.0/lib/python3.10/site-packages/requests/adapters.py", line 529, in send raise ReadTimeout(e, request=request) requests.exceptions.ReadTimeout: HTTPSConnectionPool(host='anders-test.zulipchat.com', port=443): Read timed out. (read timeout=15.0) During handling of the above exception, another exception occurred: Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/home/anders/zulip/python-zulip-api/zulip/zulip/__init__.py", line 1117, in get_server_settings return self.call_endpoint( File "/home/anders/zulip/python-zulip-api/zulip/zulip/__init__.py", line 727, in call_endpoint return self.do_api_query( File "/home/anders/zulip/python-zulip-api/zulip/zulip/__init__.py", line 665, in do_api_query raise UnrecoverableNetworkError(msg) zulip.UnrecoverableNetworkError: Connection error: Traceback (most recent call last): File "/home/anders/zulip/python-zulip-api/.direnv/python-3.10.0/lib/python3.10/site-packages/urllib3/connectionpool.py", line 445, in _make_request six.raise_from(e, None) File "<string>", line 3, in raise_from File "/home/anders/zulip/python-zulip-api/.direnv/python-3.10.0/lib/python3.10/site-packages/urllib3/connectionpool.py", line 440, in _make_request httplib_response = conn.getresponse() File "/nix/store/33jhvwkqm1327ypyhzbvl1l5d6ch62jn-python3-3.10.0/lib/python3.10/http/client.py", line 1368, in getresponse response.begin() File "/nix/store/33jhvwkqm1327ypyhzbvl1l5d6ch62jn-python3-3.10.0/lib/python3.10/http/client.py", line 317, in begin version, status, reason = self._read_status() File "/nix/store/33jhvwkqm1327ypyhzbvl1l5d6ch62jn-python3-3.10.0/lib/python3.10/http/client.py", line 278, in _read_status line = str(self.fp.readline(_MAXLINE + 1), "iso-8859-1") File "/nix/store/33jhvwkqm1327ypyhzbvl1l5d6ch62jn-python3-3.10.0/lib/python3.10/socket.py", line 705, in readinto return self._sock.recv_into(b) File "/nix/store/33jhvwkqm1327ypyhzbvl1l5d6ch62jn-python3-3.10.0/lib/python3.10/ssl.py", line 1273, in recv_into return self.read(nbytes, buffer) File "/nix/store/33jhvwkqm1327ypyhzbvl1l5d6ch62jn-python3-3.10.0/lib/python3.10/ssl.py", line 1129, in read return self._sslobj.read(len, buffer) TimeoutError: The read operation timed out During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/home/anders/zulip/python-zulip-api/.direnv/python-3.10.0/lib/python3.10/site-packages/requests/adapters.py", line 439, in send resp = conn.urlopen( File "/home/anders/zulip/python-zulip-api/.direnv/python-3.10.0/lib/python3.10/site-packages/urllib3/connectionpool.py", line 755, in urlopen retries = retries.increment( File "/home/anders/zulip/python-zulip-api/.direnv/python-3.10.0/lib/python3.10/site-packages/urllib3/util/retry.py", line 532, in increment raise six.reraise(type(error), error, _stacktrace) File "/home/anders/zulip/python-zulip-api/.direnv/python-3.10.0/lib/python3.10/site-packages/urllib3/packages/six.py", line 770, in reraise raise value File "/home/anders/zulip/python-zulip-api/.direnv/python-3.10.0/lib/python3.10/site-packages/urllib3/connectionpool.py", line 699, in urlopen httplib_response = self._make_request( File "/home/anders/zulip/python-zulip-api/.direnv/python-3.10.0/lib/python3.10/site-packages/urllib3/connectionpool.py", line 447, in _make_request self._raise_timeout(err=e, url=url, timeout_value=read_timeout) File "/home/anders/zulip/python-zulip-api/.direnv/python-3.10.0/lib/python3.10/site-packages/urllib3/connectionpool.py", line 336, in _raise_timeout raise ReadTimeoutError( urllib3.exceptions.ReadTimeoutError: HTTPSConnectionPool(host='anders-test.zulipchat.com', port=443): Read timed out. (read timeout=15.0) During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/home/anders/zulip/python-zulip-api/zulip/zulip/__init__.py", line 629, in do_api_query res = self.session.request( File "/home/anders/zulip/python-zulip-api/.direnv/python-3.10.0/lib/python3.10/site-packages/requests/sessions.py", line 542, in request resp = self.send(prep, **send_kwargs) File "/home/anders/zulip/python-zulip-api/.direnv/python-3.10.0/lib/python3.10/site-packages/requests/sessions.py", line 655, in send r = adapter.send(request, **kwargs) File "/home/anders/zulip/python-zulip-api/.direnv/python-3.10.0/lib/python3.10/site-packages/requests/adapters.py", line 529, in send raise ReadTimeout(e, request=request) requests.exceptions.ReadTimeout: HTTPSConnectionPool(host='anders-test.zulipchat.com', port=443): Read timed out. (read timeout=15.0)

I agree that reraising the error directly (raise) is a good initial solution. If we ever need a more specific error so it can be caught somewhere else, it should be raise SpecificError from e or raise SpecificError("succinct one-line message") from e (see exception chaining).

timabbott · 2021-11-22T05:07:25Z

@neiljp I was realizing it'd be helpful to have your review on this, since zulip-terminal probably cares about this aspect of error handling.

andersk · 2021-11-22T21:40:53Z

zulip/zulip/__init__.py

+                        "result": "connection-error",
+                    }
+                else:
+                    raise UnrecoverableNetworkError(msg)
            except Exception:
                # We'll split this out into more cases as we encounter new bugs.
                return {


This return also needs to be addressed.

andersk · 2021-11-22T21:42:53Z

zulip/zulip/__init__.py

@@ -356,6 +356,7 @@ def __init__(
        insecure: Optional[bool] = None,
        client_cert: Optional[str] = None,
        client_cert_key: Optional[str] = None,
+        legacy_exception_handling: Optional[bool] = False,


I don’t see any need for an option to request the old behavior. Any caller that would be modified to set this option could also be modified to handle exceptions itself.

eeshangarg · 2021-11-29T23:42:11Z

@andersk I just addressed your feedback. Thanks!

@timabbott I am honestly not sure how to write a test for this within the module. Seems like it might be better to write a test for this server-side? Should we maybe write some sort of a mocking framework that mimics a Zulip server and raises exceptions at certain points?

timabbott · 2021-11-30T22:25:39Z

A server-side test that we run alongside test-api in CI could work. If we want to write proper tests, we need to write a bit of a test framework, which is probably out of scope for this issue.

Grepping for connection-error call_on_each_event makes use of the old error handling convention, and needs to be ported to handle exceptions. We can test the ported version manually by just pointing a bot at the development environment and trying things like ctrl+C on the development server to generate connection errors.

(It's an important part of the call_on_each_event interface that it will continue running even if you get connection errors once it has started, so that your bot using that API will continue working even if the server has temporary downtime)

eeshangarg · 2022-01-21T16:24:34Z

@timabbott This PR is mostly ready for review, just needs to be tested manually. Thanks!

eeshangarg · 2022-01-21T23:25:08Z

A summary of next steps or what needs to be done:

This PR needs to be manually tested.
As for automated testing, we need to write a test framework that mimics a Zulip server to be able to test this in the same repository.
In the absence of such a test framework, a server-side test that we run in something like tools/test-api would probably work.

Thanks!

eeshangarg · 2022-01-21T23:30:28Z

Forgot to mention that the previous feedback has all been resolved on thsi PR! :)

There are cases where the call to an endpoint may result in an exception the traceback for which is converted into JSON and returned to the caller. In the case of such an unsuccessful response, we should just reraise the exception instead of parsing the response as though it was successful.

SafeConfigParser has been renamed to ConfigParser and the method SafeConfigParser.readfp() is now named ConfigParser.read_file().

timabbott · 2022-02-28T18:57:30Z

zulip/zulip/__init__.py

+                    print(f"Connection error:\n{traceback.format_exc()}")
+            except Exception:
+                if self.verbose:
+                    print(f"Unexpected error:\n{traceback.format_exc()}")


I tested this, and these needed a sleep + continue to avoid an error with res being an undefined variable when falling through here. I'll merge after applying that change.

timabbott · 2022-02-28T19:11:03Z

Merged, thanks @eeshangarg for your work on this.

zulipbot added the size: S label Nov 7, 2021

andersk reviewed Nov 13, 2021

View reviewed changes

zulipbot added size: M and removed size: S labels Nov 16, 2021

eeshangarg changed the title ~~zulip: Handle connection errors from get_server_settings properly.~~ zulip: Reraise network error exceptions. Nov 16, 2021

timabbott reviewed Nov 19, 2021

View reviewed changes

andersk reviewed Nov 22, 2021

View reviewed changes

zulipbot added size: S and removed size: M labels Nov 29, 2021

eeshangarg changed the title ~~zulip: Reraise network error exceptions.~~ zulip: Reraise exceptions in do_api_query. Nov 29, 2021

zulipbot added size: M and removed size: S labels Dec 29, 2021

andersk mentioned this pull request Jan 7, 2022

asyncio API and discord mirror (seeking feedback) #740

Open

zulipbot added the has conflicts label Feb 28, 2022

eeshangarg added 2 commits February 28, 2022 10:56

zulip: Fix deprecation warnings for SafeConfigParser and readfp.

f7afb0c

SafeConfigParser has been renamed to ConfigParser and the method SafeConfigParser.readfp() is now named ConfigParser.read_file().

timabbott reviewed Feb 28, 2022

View reviewed changes

zulipbot removed the has conflicts label Feb 28, 2022

timabbott merged commit e6dff1b into zulip:main Feb 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zulip: Reraise exceptions in do_api_query. #735

zulip: Reraise exceptions in do_api_query. #735

eeshangarg commented Nov 7, 2021

timabbott commented Nov 13, 2021

andersk Nov 13, 2021

eeshangarg Nov 13, 2021

eeshangarg Nov 13, 2021

timabbott Nov 16, 2021

eeshangarg Nov 16, 2021

timabbott Nov 19, 2021

andersk Nov 22, 2021 •

edited

Loading

eeshangarg Nov 29, 2021

timabbott commented Nov 22, 2021

andersk Nov 22, 2021

eeshangarg Nov 29, 2021

andersk Nov 22, 2021 •

edited

Loading

eeshangarg commented Nov 29, 2021

timabbott commented Nov 30, 2021

eeshangarg commented Jan 21, 2022

eeshangarg commented Jan 21, 2022

eeshangarg commented Jan 21, 2022

timabbott Feb 28, 2022

timabbott commented Feb 28, 2022

zulip: Reraise exceptions in do_api_query. #735

zulip: Reraise exceptions in do_api_query. #735

Conversation

eeshangarg commented Nov 7, 2021

timabbott commented Nov 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andersk Nov 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabbott commented Nov 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andersk Nov 22, 2021 • edited Loading

Choose a reason for hiding this comment

eeshangarg commented Nov 29, 2021

timabbott commented Nov 30, 2021

eeshangarg commented Jan 21, 2022

eeshangarg commented Jan 21, 2022

eeshangarg commented Jan 21, 2022

Choose a reason for hiding this comment

timabbott commented Feb 28, 2022

andersk Nov 22, 2021 •

edited

Loading

andersk Nov 22, 2021 •

edited

Loading