Adding request timeout #157

lugino-emeritus · 2017-10-16T11:18:48Z

Adding a request timeout in method _send in api.py
- timeout is 10 + query_params.get("timeout", 30000) / 1000 -> OK? IMHO it shouldn't be constant, since it has to be larger than a possible server side timeout
Handle possible exceptions in listen_forever

This should solve #129 and #131

requests exception handling in listen_forever

update to matrix-org master

bad_sync_timeout was initialized with 5000 (seconds), should be only 5 seconds.

simonklb · 2017-10-16T13:37:34Z

There should probably be some form of retry limit so that it doesn't hang forever if a server is/goes down.

lugino-emeritus · 2017-10-16T16:17:48Z

My basic idea was: when using this sdk do design a bot, and the server is offline even for a few hours the client should reconnect again.

So maybe the following is usefull: Option to define a maximal time until listen_forever would be stopped if it is not successful.

But then we have to discuss the following:

How should we stop listen_forever, i.e. what exception is useful? Reraise the last exception? Throw a new exception of type MatrixError? Define a new exception?
Which default value for this time is useful? 6 hours? Or endless (to be compatible to old versions)?

And also (design):

should it be possible to define this time in listen_forever?
or use a new variable without a separate method to set this variable (like bad_sync_timeout_limit)?
or with a separate method to set this variable?

I would prefer the following:

add variable max_retry_time, init it in init with 6 * 60 * 60 = 6 hours
raise MatrixError("Stoped listen_forever, since reconnection was not possible for %i seconds") to stop listen_forever

simonklb · 2017-10-16T17:21:42Z

Although reconnect/retry mechanisms are very useful and user-friendly I don't think this kind of logic belong in the SDK. If you want a bot that automatically reconnects when the server comes back online you should add some kind of heartbeat functionality in the bot and not force it on everyone else using the SDK.

If it's decided to include this in the SDK please make it optional and make sure exceptions still are able to bubble up to the client implementations.

lugino-emeritus · 2017-10-16T18:04:30Z

Well, currently listen_forever is running endless, even if the server is not reachable. So I just did things to be compatible with old versions of this sdk.

I did this pull request for a timeout in request, to make sure that requests.request is not running endless. (See #129)

And I added the handling of the new timeout Errors because of the second comment in #149.

Assume you are running listen_forever, and your router is changing the ip (a lot routers do that all 3 days). Now, do you want that the sdk is reconnecting again or not? I only changed things for that situation.
I didn't changed the case when the server is offline, see here.

So adding a max_retry_time (or something like that) would be a new feature, and IMHO that should be handled in a different pull request / issue.

simonklb · 2017-10-17T07:01:07Z

I don't mind the timeout. I just don't want to have auto-reconnect logic in the SDK. At the least the retry should be optional and have a set limit.

If you only added the request timeout everything would be fine by me. But you also decided to catch ConnectionError and ReadTimeout which is going to prevent implementations from catching those using the exception_handler now.

lugino-emeritus · 2017-10-17T14:04:53Z

I agree with you, the exception_handler should handle those exceptions. But then it should also handle the MatrixRequestError (if it is not None). This exception would be thrown if synapse is down, but the server is online. And this is not nice (could be an endless loop). Even more the exception handler should know the time of the last successful connection (or maybe more informations in future).

My suggestion:

remove the changes in listen_forever
do a new pull request with a new / additional exception_handler (to be compatible to old versions)

original client.py

listen_forever with new exception handler

simonklb · 2017-10-19T12:11:16Z

If Synapse is down you shouldn't get any response, a ConnectionError exception should be thrown, so I'm not sure what you mean by that. The MatrixRequestError exceptions are thrown if you get a response that does not have a response code between 200 and 300, i.e. some error in the Matrix server.

Are you running some form of proxy in-front of Synapse?

lugino-emeritus · 2017-10-20T18:17:06Z

Yes, I'm running nginx as proxy. And I get a 502 'Bad Gateway' error. Makes sense, of course. However, I implemented a new exception handler. Of course it would be prettier to remove the old exception handler. But then it would not be compatible to old versions. What do you think, should I do a pull request? But first this one should be merged.

simonklb · 2017-10-21T11:28:52Z

That explains the confusion. The problem here is not that listener isn't catching the exceptions correctly but rather that the API assumes that the HTTP errors are generated by the Matrix server and not something else (the nginx proxy in your case).

@non-Jedi @Half-Shot any ideas?

non-Jedi · 2017-10-22T19:07:58Z

Okay. So I haven't read the actual changes yet, so I'm not commenting on that,
yet. Per the above conversation, we should have two kinds of exceptions:

MatrixRequestError for any status codes outside of the acceptable range. We
shouldn't differentiate between status codes returned by synapse and any
other random server.
A new MatrixTimeoutError (or some other name) subclassing MatrixError for
this new situation where a request doesn't return within specified timeout.

listen_forever should probably just have a default of propagating errors
upward with an option of handling the errors and retrying. That would be my
preference. If anyone currently depends on the method just silently swallowing
errors, they can opt into that behavior since it should definitely be opt-in
behavior.

Any other questions I missed?

non-Jedi · 2017-10-22T19:09:02Z

@lugino-emeritus could you squash these commits and sign-off per
Contributing.rst, please? Thanks.

non-Jedi

Default timeout should be user-tunable for every method of MatrixHttpApi.
Either a new kwarg on every method or a default_timeout kwarg on instantiation
of the object.

I'm also not sure 10 seconds is the correct default value. 30 seconds might make
more sense.

non-Jedi · 2017-10-22T19:16:21Z

matrix_client/api.py

@@ -582,6 +582,8 @@ def _send(self, method, path, content=None, query_params={}, headers={},

        if headers["Content-Type"] == "application/json" and content is not None:
            content = json.dumps(content)
+
+        request_timeout = 10.0 + query_params.get("timeout", 30000) / 1000


Any particular reason we're forcing this to be a float instead of an int?

no. It is just because the requests_timeout could be a float. I will change it.

non-Jedi · 2017-10-22T19:19:51Z

matrix_client/api.py

@@ -590,7 +592,8 @@ def _send(self, method, path, content=None, query_params={}, headers={},
                params=query_params,
                data=content,
                headers=headers,
-                verify=self.validate_cert
+                verify=self.validate_cert,
+                timeout=request_timeout


Whatever error is raised by requests for a timeout needs to be handled and
reraised as a subclass of MatrixError. (maybe a new error class
MatrixTimeoutError?)

Should we also handle the ConnectTimeout next to the ReadTimeout here? With the same exception class? Maybe the ConnectTimeout should be handled with this pull request.

simonklb · 2017-10-23T08:56:24Z

@non-Jedi I agree with you that making errors propagate is the best course of action. Also, trying to do something fancy to distinguish between actual Matrix server errors and errors spawned by an ingress service or such might be tricky.

But, since HTTP errors are directly translated into Matrix errors rather than kept as request exceptions it might be worth doing something to avoid this kind of confusion. A short mention in the MatrixRequestError doc? Right now it simply says "The home server returned an error response." which obviously wasn't the case here... :)

non-Jedi · 2017-10-23T16:17:42Z

@simonklb would absolutely be a good thing, and I would accept a PR for that immediately. :)

Signed-off-by: Niklas Tittjung <[email protected]>

Just have to learn some github things... Sorry for changing this file temporary

non-Jedi

Other than that, it LGTM. Fix those things, squash your commits, and rebase on current master, and we'll see if we can get this one merged before the next release. :)

non-Jedi · 2017-11-02T00:41:00Z

matrix_client/api.py

+                    verify=self.validate_cert,
+                    timeout=request_timeout
+                )
+            except requests.exceptions.ReadTimeout:


Catch requests.exceptions.Timeout here instead.

non-Jedi · 2017-11-02T00:41:37Z

matrix_client/api.py

@@ -582,16 +582,24 @@ def _send(self, method, path, content=None, query_params={}, headers={},

        if headers["Content-Type"] == "application/json" and content is not None:
            content = json.dumps(content)
+
+        request_timeout = 30 + query_params.get("timeout", 30000) / 1000


Add a new kwarg default_timeout=25 to __init__, and then do:

request_timeout = 5 + query_params.get("timeout", self.default_timeout * 1000) / 1000

non-Jedi · 2017-11-02T00:44:45Z

matrix_client/api.py

+                    timeout=request_timeout
+                )
+            except requests.exceptions.ReadTimeout:
+                raise MatrixTimeoutError(


Pass the requests exception into the constructor for MatrixTimeoutError
so that people can debug easier. Also consider passing in the full URL that
was called and the method to be inserted into the error's msg (See
MatrixRequestError for an example of this).

update to matrix-org/master

solved bug in MatrixHttpLibError

Signed-off-by: Niklas Tittjung [email protected]

request_timeout to master

master to request_timeout

Signed-off-by: Niklas Tittjung [email protected]

update from matrix.org

Signed-off-by: Niklas Tittjung [email protected]

lugino-emeritus added 7 commits October 4, 2017 14:18

add timeout to requests.request

7dd6a4a

handle requests timeout in listen_forever

1d915e2

import ReadTimeout from requests.exceptions

98f9ce8

Update client.py

83b5047

requests exception handling in listen_forever

Merge pull request #1 from matrix-org/master

b73f4ad

update to matrix-org master

sync timeout correction

545ade3

bad_sync_timeout was initialized with 5000 (seconds), should be only 5 seconds.

Update client.py

8365a37

lugino-emeritus added 3 commits October 17, 2017 16:10

Update client.py

456ed7f

Merge pull request #2 from lugino-emeritus/request_timeout

1acce6a

original client.py

Update client.py

25ac7f2

listen_forever with new exception handler

lugino-emeritus mentioned this pull request Oct 18, 2017

Add handling for exceptions raised by requests library #123

Merged

non-Jedi requested changes Oct 22, 2017

View reviewed changes

non-Jedi added the Awaiting Changes label Oct 22, 2017

lugino-emeritus and others added 4 commits October 23, 2017 18:24

Adding MatrixTimeoutError

561e59a

Signed-off-by: Niklas Tittjung <[email protected]>

import MatrixTimeoutError

1be8337

Signed-off-by: Niklas Tittjung <[email protected]>

Merge branch 'develop' into master

fd18039

Update client.py

eba1de2

Just have to learn some github things... Sorry for changing this file temporary

non-Jedi requested changes Nov 2, 2017

View reviewed changes

lugino-emeritus added 23 commits November 2, 2017 16:58

restore api.py (temp)

7214851

Merge pull request #4 from matrix-org/master

6c762cb

update to matrix-org/master

Update errors.py

ef1621d

Update errors.py

9000528

Update api.py

1115d03

correct bad_sync_timeout

1280a99

insert empty line

c90d401

update MatrixTimeoutError

c7d57b9

solved bug in MatrixHttpLibError

edit MatrixTimeoutError

0f88e26

Merge branch 'master' into request_timeout_update

0fcd77b

Update code for request timeout

d3c35da

Signed-off-by: Niklas Tittjung [email protected]

pep corrections

73b4341

pep corrections

81ea5bd

some pep corrections

fd4692b

Signed-off-by: Niklas Tittjung [email protected]

Update api.py

93f573c

removed whitespace

206e2a0

remove blank line

5464f7a

Signed-off-by: Niklas Tittjung [email protected]

Merge branch 'master' into master

1e465f8

Merge pull request #9 from lugino-emeritus/master

7f70e5c

request_timeout to master

Merge pull request #10 from matrix-org/master

021e7dd

master to request_timeout

Update lugino-emeritus/master from matrix-org/master

18d1824

Signed-off-by: Niklas Tittjung [email protected]

Merge pull request #12 from matrix-org/master

0712ced

update from matrix.org

Update to current version (2018-03-07)

7523266

Signed-off-by: Niklas Tittjung [email protected]

non-Jedi added enhancement bug and removed enhancement labels May 15, 2018

non-Jedi self-assigned this May 21, 2018

jahschwa mentioned this pull request May 12, 2019

matrix: audit timeouts jahschwa/sibyl#62

Open

3nprob mentioned this pull request Aug 21, 2021

TCP timeout #129

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding request timeout #157

Adding request timeout #157

lugino-emeritus commented Oct 16, 2017

simonklb commented Oct 16, 2017

lugino-emeritus commented Oct 16, 2017 •

edited

Loading

simonklb commented Oct 16, 2017

lugino-emeritus commented Oct 16, 2017

simonklb commented Oct 17, 2017

lugino-emeritus commented Oct 17, 2017

simonklb commented Oct 19, 2017

lugino-emeritus commented Oct 20, 2017

simonklb commented Oct 21, 2017

non-Jedi commented Oct 22, 2017

non-Jedi commented Oct 22, 2017

non-Jedi left a comment •

edited

Loading

non-Jedi Oct 22, 2017

lugino-emeritus Oct 23, 2017

non-Jedi Oct 22, 2017

lugino-emeritus Oct 23, 2017

simonklb commented Oct 23, 2017

non-Jedi commented Oct 23, 2017

non-Jedi left a comment

non-Jedi Nov 2, 2017

non-Jedi Nov 2, 2017

non-Jedi Nov 2, 2017

Adding request timeout #157

Are you sure you want to change the base?

Adding request timeout #157

Conversation

lugino-emeritus commented Oct 16, 2017

simonklb commented Oct 16, 2017

lugino-emeritus commented Oct 16, 2017 • edited Loading

simonklb commented Oct 16, 2017

lugino-emeritus commented Oct 16, 2017

simonklb commented Oct 17, 2017

lugino-emeritus commented Oct 17, 2017

simonklb commented Oct 19, 2017

lugino-emeritus commented Oct 20, 2017

simonklb commented Oct 21, 2017

non-Jedi commented Oct 22, 2017

non-Jedi commented Oct 22, 2017

non-Jedi left a comment • edited Loading

Choose a reason for hiding this comment

non-Jedi Oct 22, 2017

Choose a reason for hiding this comment

lugino-emeritus Oct 23, 2017

Choose a reason for hiding this comment

non-Jedi Oct 22, 2017

Choose a reason for hiding this comment

lugino-emeritus Oct 23, 2017

Choose a reason for hiding this comment

simonklb commented Oct 23, 2017

non-Jedi commented Oct 23, 2017

non-Jedi left a comment

Choose a reason for hiding this comment

non-Jedi Nov 2, 2017

Choose a reason for hiding this comment

non-Jedi Nov 2, 2017

Choose a reason for hiding this comment

non-Jedi Nov 2, 2017

Choose a reason for hiding this comment

lugino-emeritus commented Oct 16, 2017 •

edited

Loading

non-Jedi left a comment •

edited

Loading