blockworks-foundation / mango-explorer

A project to explore and provide useful code for Mango Markets (https://mango.markets/).
MIT License
155 stars 87 forks source link

Adding option to define HTTP timeout for POST requests to RPC nodes #30

Closed ochaloup closed 2 years ago

ochaloup commented 2 years ago

We do experience a trouble when HTTP request is stuck. The mango explorer market maker is then hanging forever at the requests.post call. The expected processing is that the request is timeouted and processing switches to the next node in the list. This PR proposes a change bringing such a behavior.

@OpinionatedGeek WDYT?

OpinionatedGeek commented 2 years ago

Great idea! Thanks for tackling this.

Unfortunately I made a change last night that introduced a conflict. Sorry. (It's an easy fix though.)

In general, I'd prefer to make it consistent with the existing approach. (It's nearly consistent, but not quite.) The general configuration approach is: there's a parameter for the command line, the merging of the command line parameter value and the default happens in ContextBuilder.build(), and the actual runtime objects have specific values rather than None.

(I realise this isn't documented anywhere. Sorry.)

So: ContextBuilder.build() takes in a bunch of typing.Optional[]s and a set of defaults, and produces the fixed configuration values to use.

With this approach, RPCCaller would take a float, not a typing.Optional[float], and we'd have to come up with our own default value for the timeout.

How does this sound to you? And what do you think the default timeout should be? (I also like the idea of our choice of timeout to be explicit, since we know we're working with RPC servers and might prefer a different default than the regular requests libraries default.)

ochaloup commented 2 years ago

@OpinionatedGeek yes, I undestand the approach just I did so intentionally. I was thinking the default value for timeout should be None - i.e., do not timeout by default - and that's why I chosen the Optional. https://docs.python-requests.org/en/latest/user/quickstart/#timeouts - If no timeout is specified explicitly, requests do not time out. I'm not able to find what timeout = 0 means but it's around to timeout immediately (kind of discussion https://github.com/psf/requests/issues/1615). As well the default value of timeout is None in request method in http python library.

How to work with default value of None that I would like to pass as the value? Or should there be used some arbitrary number rather than None - like let's say 20 sec?

A note to myself: I should be catching the ReadTimeout apart from the ConnectionTimeout.

OpinionatedGeek commented 2 years ago

That is an excellent point. Hmm.

I do still think we should generally have a short(er) timeout than a regular HTTP request. But as you say it should still be possible to set an unlimited timeout.

The most common use-cases of mango-explorer seem to be in long-running processes like marketmakers, and setting no timeout would be bad in that situation.

On the other hand, some RPC server operations can take a very long time. (In particular some getProgramAccounts() calls can take over a minute.)

There is precedent in some of the other code for using -1 in a command-line parameter to disable a function/feature. I'm not especially keen on it, but maybe it's worth thinking about here? A default value of 10 or 20 seconds, the ability to specify it on the command-line, the RPCCaller taking a float rather than a typing.Optional[float], and -1 as setting no timeout.

What are your thoughts?

ochaloup commented 2 years ago

@OpinionatedGeek +1. I pushed the changes, it's ready for review.

ochaloup commented 2 years ago

@OpinionatedGeek I updated the PR for not containing the merge conflicts. It's ready for review. Thanks.

OpinionatedGeek commented 2 years ago

Brilliant! Thanks for doing this.