Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

provide a default timeout on http client #1405

Merged
merged 1 commit into from
Oct 4, 2023
Merged

provide a default timeout on http client #1405

merged 1 commit into from
Oct 4, 2023

Conversation

jtarchie
Copy link
Contributor

@jtarchie jtarchie commented Oct 3, 2023

When using the rqlite CLI, I could not determine an issue of connection. It turns out that the arguments I had provided were not working. The issue was not present until I added a timeout to the HTTP client.

@jtarchie jtarchie changed the title provide a timeout on http client provide a default timeout on http client Oct 3, 2023
@otoolep
Copy link
Member

otoolep commented Oct 3, 2023

Thanks for your contribution. Can you show me what you passed to rqlite? Normally it exits immediately if it can't connect.

I do think your change makes sense.

cmd/rqlite/main.go Outdated Show resolved Hide resolved
@jtarchie
Copy link
Contributor Author

jtarchie commented Oct 3, 2023

Thanks for your contribution. Can you show me what you passed to rqlite? Normally it exits immediately if it can't connect.

I do think your change makes sense.

I assumed when I passed -s https that it would switch to the correct port 443. When I added the timeout, I finally saw the error message of the scheme://hostname:port and realized what was missing. I understand why the port wasn't changed now.

@otoolep
Copy link
Member

otoolep commented Oct 3, 2023

@jtarchie -- were you running an rqlited process at the time? If so, what host and port was it listening on? When I try to reproduce this issue I get:

rqlite -s https
ERR! Get "https://127.0.0.1:4001//status": http: server gave HTTP response to HTTPS client

and if nothing is running I get:

rqlite -s https 
ERR! Unable to connect to rqlited at https://127.0.0.1:4001 - is it running?

@jtarchie
Copy link
Contributor Author

jtarchie commented Oct 3, 2023

Your example above is when it is run locally. The networking configuration for your host machine might account for timeouts, such as firewalls just dropping packets to any port not being listened on.

For example,

 2023-10-03 06:54:50 ⌚  ~/workspace/rqlite
± |main ✓| → nc localhost 8811

 2023-10-03 06:55:09 ⌚  ~/workspace/rqlite
± |main ✓| → nc localhost 8811 -v
nc: connectx to localhost port 8811 (tcp) failed: Connection refused
nc: connectx to localhost port 8811 (tcp) failed: Connection refused

 2023-10-03 06:55:22 ⌚  ~/workspace/rqlite
± |main ✓| → nc google.com 8811 -v
# this just hangs

 2023-10-03 06:57:13 ⌚  ~/workspace/rqlite
± |main ✓| → nc google.com 8811 -v --apple-tcp-timeout 2
nc: connectx to google.com port 8811 (tcp) failed: Operation timed out
# times out after 2 seconds

I was running through GCP, behind a load balancer I don't control. This is a reproducible example if you are so inclined.

@otoolep
Copy link
Member

otoolep commented Oct 3, 2023

Ack, thanks, I'll merge this (or something very similar) soon.

@jtarchie
Copy link
Contributor Author

jtarchie commented Oct 3, 2023

I can make changes if you want. Please let me know.

TLSClientConfig: &tls.Config{InsecureSkipVerify: argv.Insecure, RootCAs: rootCAs},
Proxy: http.ProxyFromEnvironment,
},
Timeout: httpTimeout,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the timeout be configurable like the insecure option too? e.g 30s or something that can be parsed with time.ParseDuration

@otoolep
Copy link
Member

otoolep commented Oct 4, 2023

I thought that might be overkill, but the Go docs say this timeout affects all HTTP requests. So if retrieving backup took longer than the timeout (quite possible) it might break that.

So yeah, let's add a timeout option. It would be:

-t 10s

Default to 30s?

When using the `rqlite` CLI, I was unable to determine an issue of connection.
It turns out that the arugments I had provided were not working.
The issue did not present itself till I added a timeout to the HTTP client.
@jtarchie
Copy link
Contributor Author

jtarchie commented Oct 4, 2023

I used the built-in functionality of the CLI library. It can parse durations from the command line arguments.

@otoolep otoolep merged commit b730142 into rqlite:master Oct 4, 2023
1 of 2 checks passed
@otoolep
Copy link
Member

otoolep commented Oct 4, 2023

Thanks!

otoolep added a commit that referenced this pull request Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants