Skip to content
This repository has been archived by the owner on Feb 13, 2024. It is now read-only.

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

Merged

Conversation

ochaloup
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

ochaloup commented Jan 20, 2022

@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 psf/requests#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
Copy link
Contributor

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
Copy link
Contributor Author

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

@ochaloup
Copy link
Contributor Author

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

@OpinionatedGeek OpinionatedGeek merged commit 5d36d18 into blockworks-foundation:main Jan 24, 2022
@OpinionatedGeek
Copy link
Contributor

Brilliant! Thanks for doing this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants