Skip to content

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# Changelog

## v4.2.0 - 2025-02-25

- Added the `timeout` configuration option.

## v4.1.0 - 2025-02-08

- Added the `follow_redirects` configuration option.
Expand Down
2 changes: 1 addition & 1 deletion gleam.toml
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"]
Expand Down
23 changes: 21 additions & 2 deletions src/gleam/httpc.gleam
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

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 too

Copy link
Member

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

Copy link
Author

@caifanuncle caifanuncle Feb 28, 2025

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 or RequestTimeout.

(source: admittedly chatgpt lol)

Python (requests & httpx)
requests.exceptions.Timeout → RequestTimeout
httpx.TimeoutException → HttpxTimeoutError
JavaScript (Axios, Node fetch)
AxiosError: timeout exceeded → AxiosTimeoutError
AbortError (used in fetch) → FetchTimeoutError
Go (net/http, fasthttp)
net/http: request canceled (Client.Timeout exceeded) → RequestCanceledError
fasthttp.ErrTimeout → FastHttpTimeoutError
Ruby (Net::HTTP, Faraday)
Net::OpenTimeout → OpenTimeoutError
Net::ReadTimeout → ReadTimeoutError
Faraday::TimeoutError → FaradayTimeoutError
Rust (reqwest)
reqwest::Error::TimedOut → ReqwestTimeoutError
Java (HttpClient, OkHttp)
java.net.SocketTimeoutException → SocketTimeoutError
java.net.ConnectException → ConnectTimeoutError
okhttp3.internal.http2.StreamResetException → OkHttpTimeoutError

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?

Copy link
Member

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.

Copy link
Author

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

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 existing InvalidUtf8Response 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.

Copy link
Member

Choose a reason for hiding this comment

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

"ResponseTimedOut"? To match "FailedToConnect"?

}

pub type ConnectError {
Expand All @@ -27,6 +31,7 @@ fn normalise_error(error: Dynamic) -> HttpError
type ErlHttpOption {
Ssl(List(ErlSslOption))
Autoredirect(Bool)
Timeout(Int)
}

type BodyFormat {
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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),
Copy link
Member

Choose a reason for hiding this comment

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

Document what the default behaviour is please

Copy link
Member

Choose a reason for hiding this comment

The 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)
Copy link
Author

@caifanuncle caifanuncle Feb 25, 2025

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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.
Expand All @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The 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(
Expand Down
2 changes: 2 additions & 0 deletions src/gleam_httpc_ffi.erl
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ normalise_error(Error = {failed_connect, Opts}) ->
_ -> erlang:error({unexpected_httpc_error, Error})
end,
{failed_to_connect, normalise_ip_error(Ipv4), normalise_ip_error(Ipv6)};
normalise_error(timeout) ->
request_timeout;
normalise_error(Error) ->
erlang:error({unexpected_httpc_error, Error}).

Expand Down
29 changes: 29 additions & 0 deletions test/gleam_httpc_test.gleam
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
Expand Down Expand Up @@ -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)
}
Copy link
Member

Choose a reason for hiding this comment

The 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.