Add --timeout option and a connect_timeout config default - #1622
Conversation
pgcli never sets a connection timeout, so it inherits libpq's default of 0: wait until the operating system gives up on the TCP connection. Against an address that swallows SYNs, pgcli keeps hanging for minutes with no feedback. Adds a --timeout command line option and a connect_timeout config value (default 30 seconds). Precedence, highest first: 1. --timeout 2. connect_timeout in the connection string 3. $PGCONNECT_TIMEOUT 4. the config value Points 2 and 3 keep libpq's own ordering, and when only the environment variable is set nothing is injected, so libpq reads it itself. --timeout 0 is meaningful (wait forever) and is not treated as unset. The resolved value is passed as a connection parameter rather than merged into the dsn, so the user's connection string is untouched; PGExecute keeps it alongside the dsn the same way it keeps hostaddr. Adds seven tests covering each precedence combination. Two existing tests asserted the exact PGExecute call and now include the resolved timeout.
| in_dsn = "connect_timeout" in conninfo_to_dict(dsn) if dsn else False | ||
| if not in_dsn and "connect_timeout" not in kwargs and not os.environ.get("PGCONNECT_TIMEOUT"): | ||
| try: | ||
| timeout = int(self.default_connect_timeout) |
There was a problem hiding this comment.
I'd rather see this conversion when setting self.default_connect_timeout in __init__(). That way, all uses of the attribute can be sure that it's an integer (and not garbage). We could use this:
self.default_connect_timeout = c["main"].as_int("default_connect_timeout")
That will (1) convert to an integer; (2) automatically default to 30 (from pgclirc) if the user removed the line from their configuration file; and (3) raise an error if the user configured a non-integer value, which I find useful: I prefer when the software says I have done something stupid, instead of silently ignoring it, and making me search and finally find out that, yes, I did something stupid. ;)
| if new_params["dsn"]: | ||
| # When using DSN, only keep dsn, password, and hostaddr (for SSH tunnels) | ||
| new_params = {k: v for k, v in new_params.items() if k in ("dsn", "password", "hostaddr")} | ||
| new_params = {k: v for k, v in new_params.items() if k in ("dsn", "password", "hostaddr", "connect_timeout")} |
There was a problem hiding this comment.
The comment above is now out-of-date.
| assert get_editor() is None | ||
|
|
||
|
|
||
| def _effective_connect_timeout(tmpdir, cli_timeout=None, dsn_timeout=None, env=None, cfgval=None): |
There was a problem hiding this comment.
I guess it works... but a get_connect_timeout(dsn, default, kwargs) helper method would probably be easier to unit-test. What do you think?
2 participants
Description
pgcli never sets a connection timeout, so it inherits libpq's default of
0,which means "wait until the operating system gives up on the TCP connection".
That can take minutes with no feedback.
Measured against
192.0.2.1(TEST-NET, silently drops SYNs):That is a poor default for an interactive client: an unreachable host, a
bastion that is down or a wrong port all look like a hang.
Change
Adds a
--timeoutcommand line option and aconnect_timeoutconfig value(default 30 seconds). Precedence, highest first:
--timeoutconnect_timeoutin the connection string$PGCONNECT_TIMEOUTconnect_timeoutconfig valuePoints 2 and 3 keep libpq's own ordering, and when only the environment
variable is set, nothing is injected so libpq reads it itself.
--timeout 0is meaningful (wait forever, the previous behaviour) and is not treated as
unset, so nobody is locked out of the old default.
The resolved value is passed as a connection parameter rather than merged into
the dsn, so the user's connection string reaches
PGExecuteexactly aswritten.
PGExecutekeepsconnect_timeoutalongside the dsn the same way italready keeps
hostaddr.Validation
Seven new tests in
tests/test_main.pycovering every precedence combination:config default, config value, connection string over config, connection string
over env, env left to libpq,
--timeoutover everything, and--timeout 0.Two existing tests (
test_pg_service_file,test_application_name_db_uri)assert the exact
PGExecutecall and now include the resolved timeout.Measured end to end against the same unreachable address:
connect_timeout=5in the connection stringPGCONNECT_TIMEOUT=4--timeout 2(with 20 in the string and 9 in the env)Full suite green locally (2737 passed).
Checklist
changelog.rst.Part of the feature list in discussion #1603: this is item 23.