-
Notifications
You must be signed in to change notification settings - Fork 19
Add custom timeout option for requests #45
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
name = "gleam_httpc" | ||
version = "4.1.0" | ||
version = "4.2.0" | ||
gleam = ">= 1.0.0" | ||
|
||
licences = ["Apache-2.0"] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,12 +5,16 @@ import gleam/http.{type Method} | |
import gleam/http/request.{type Request} | ||
import gleam/http/response.{type Response, Response} | ||
import gleam/list | ||
import gleam/option.{type Option, None, Some} | ||
import gleam/result | ||
import gleam/uri | ||
|
||
pub type HttpError { | ||
InvalidUtf8Response | ||
FailedToConnect(ip4: ConnectError, ip6: ConnectError) | ||
/// The response body was not received within the configured timeout period | ||
/// | ||
RequestTimeout | ||
} | ||
|
||
pub type ConnectError { | ||
|
@@ -27,6 +31,7 @@ fn normalise_error(error: Dynamic) -> HttpError | |
type ErlHttpOption { | ||
Ssl(List(ErlSslOption)) | ||
Autoredirect(Bool) | ||
Timeout(Int) | ||
} | ||
|
||
type BodyFormat { | ||
|
@@ -106,7 +111,10 @@ pub fn dispatch_bits( | |
|> uri.to_string | ||
|> charlist.from_string | ||
let erl_headers = prepare_headers(req.headers) | ||
let erl_http_options = [Autoredirect(config.follow_redirects)] | ||
let erl_http_options = case config.timeout { | ||
None -> [Autoredirect(config.follow_redirects)] | ||
Some(timeout) -> [Autoredirect(config.follow_redirects), Timeout(timeout)] | ||
} | ||
let erl_http_options = case config.verify_tls { | ||
True -> erl_http_options | ||
False -> [Ssl([Verify(VerifyNone)]), ..erl_http_options] | ||
|
@@ -155,13 +163,18 @@ pub opaque type Configuration { | |
/// Whether to follow redirects. | ||
/// | ||
follow_redirects: Bool, | ||
/// Timeout for the request in milliseconds. | ||
/// This defaults to `Infinity`, meaning the request will not raise a timeout error | ||
/// unless you provide a timeout value. | ||
/// | ||
timeout: Option(Int), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Document what the default behaviour is please There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you document what the timeout is for please? I think it's the timeout for the entire response body to be received, starting from when the request is sent. Remove the infinity part as there's no such value in Gleam. It would be good to have a default timeout, as @jakecleary says |
||
) | ||
} | ||
|
||
/// Create a new configuration with the default settings. | ||
/// | ||
pub fn configure() -> Configuration { | ||
Builder(verify_tls: True, follow_redirects: False) | ||
Builder(verify_tls: True, follow_redirects: False, timeout: None) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. default timeout on Erlang implementation is Infinity (ie no timeout) according to https://www.erlang.org/docs/26/man/httpc#request-5 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to mirror the erlang implementation or have a more "sensible" default, like say 30s/60s? I've only worked with C#/php over the years but haven't seen an infinite timeout before. Dotnet uses 100s for example. It's ultimately arbitrary, but an infinite default feels like a bit of a trap to me, ha. |
||
} | ||
|
||
/// Set whether to verify the TLS certificate of the server. | ||
|
@@ -182,6 +195,12 @@ pub fn follow_redirects(config: Configuration, which: Bool) -> Configuration { | |
Builder(..config, follow_redirects: which) | ||
} | ||
|
||
/// Set the timeout for the request in milliseconds. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Document what the default behaviour is if this isn't called please |
||
/// The default timeout value is `Infinity`, meaning the request will not raise a timeout error unless this is called | ||
pub fn set_timeout(config: Configuration, timeout: Int) -> Configuration { | ||
Builder(..config, timeout: Some(timeout)) | ||
} | ||
|
||
/// Send a HTTP request of unicode data. | ||
/// | ||
pub fn dispatch( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
import gleam/erlang/atom | ||
import gleam/http.{Get, Head, Options} | ||
import gleam/http/request | ||
import gleam/http/response | ||
|
@@ -133,3 +134,31 @@ pub fn custom_user_agent_test() { | |
httpc.send(request.set_header(req, "user-agent", "gleam-test")) | ||
let assert True = string.contains(resp.body, "\"User-Agent\": \"gleam-test") | ||
} | ||
|
||
pub fn timeout_success_test() { | ||
let req = | ||
request.new() | ||
|> request.set_method(Get) | ||
|> request.set_host("httpbin.org") | ||
|> request.set_path("/delay/1") | ||
|
||
let assert Ok(resp) = | ||
httpc.configure() | ||
|> httpc.set_timeout(5000) | ||
|> httpc.dispatch(req) | ||
|
||
let assert 200 = resp.status | ||
} | ||
|
||
pub fn timeout_error_test() { | ||
let req = | ||
request.new() | ||
|> request.set_method(Get) | ||
|> request.set_host("httpbin.org") | ||
|> request.set_path("/delay/1") | ||
|
||
let assert Error(httpc.RequestTimeout) = | ||
httpc.configure() | ||
|> httpc.set_timeout(200) | ||
|> httpc.dispatch(req) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Split this into two tests please and use the shortest possible timeouts rather than thes long ones. We don't want the tests to take a long time to run. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we improve this name? I'd like to make it clear that the timeout is that the response body wasn't received in time. Documenting it with a
///
comment would be useful tooThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was asking what ideas you had for names 😁 I wonder what other HTTP clients call this error
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did a bit research on http clients from other languages, and i find that the vocabulary they use for this type of error tends to be either a simple
Timeout
orRequestTimeout
.(source: admittedly chatgpt lol)
tbh i still think a simple
RequestTimeout
is more than descriptive as ppl tend to see the request time as a whole, from the moment the request is triggered to the moment the response body is received in full. wdyt?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the LLM is hallucinating or misunderstood the question there. The ones I checked didn't exist and some of them don't seem to make any sense for how the language works?
Please don't share LLM output as research. Following this fake information is a waste of everybody's time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apologies, just thought it could be a quick way to get some inspiration. will be mindful of this in the future. 🙏
did some 'proper' research this time round and i have the following. not sure which set of vocabulary resonates most w u; i still personally think RequestTimeout is simple and descriptive enough.
WDYT?
NodeJS
Request Timeout
Ruby (Net)
Read Timeout
Python
requests.exceptions.Timeout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey 👋 Thought I would chime in...
I think framing it from the perspective of the request is more common (i.e. some variant of
RequestTimeout
) but I like the idea of mirroring the existingInvalidUtf8Response
error and having something like:NoResponseReceived
.I quite like how explicit that is, but it might feel a bit less familiar, coming from other languages. e.g. coming from C#/dotnet, it uses a
TimeoutException
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"ResponseTimedOut"? To match "FailedToConnect"?