~emersion/hut-dev

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
3 2

[PATCH] Make timeout configurable

Details
Message ID
<20220516150540.42563-1-blallo@autistici.org>
DKIM signature
pass
Download raw message
Patch: +15 -4
The http client used for interacting with the hut instance had a
hardcoded 30 seconds timeout. While this might be ok in most cases, I
happened to hit one of the corner cases when trying to push a tarball to
site.ht. This commit adds a global flag to set the value, defaulting to
the old one.
---
 config.go | 18 ++++++++++++++----
 main.go   |  1 +
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/config.go b/config.go
index 1f9c786..fa0b8e3 100644
--- a/config.go
+++ b/config.go
@@ -114,6 +114,11 @@ func createClient(service string, cmd *cobra.Command) *Client {
}

func createClientWithInstance(service string, cmd *cobra.Command, instanceName string) *Client {
	timeout, err := cmd.Flags().GetDuration("timeout")
	if err != nil {
		log.Fatal(err)
	}

	customConfigFile := true
	configFile, err := cmd.Flags().GetString("config")
	if err != nil {
@@ -184,14 +189,14 @@ func createClientWithInstance(service string, cmd *cobra.Command, instanceName s
	if baseURL == "" {
		log.Fatalf("failed to get origin for service %q in instance %q", service, inst.Name)
	}
	return createClientWithToken(baseURL, token)
	return createClientWithToken(baseURL, token, timeout)
}

func createClientWithToken(baseURL, token string) *Client {
func createClientWithToken(baseURL, token string, timeout time.Duration) *Client {
	gqlEndpoint := baseURL + "/query"
	tokenSrc := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: token})
	httpClient := oauth2.NewClient(context.Background(), tokenSrc)
	httpClient.Timeout = 30 * time.Second
	httpClient.Timeout = timeout
	return &Client{
		Client:  gqlclient.New(gqlEndpoint, httpClient),
		BaseURL: baseURL,
@@ -233,6 +238,11 @@ func newInitCommand() *cobra.Command {
	cmd.Run = func(cmd *cobra.Command, args []string) {
		ctx := cmd.Context()

		timeout, err := cmd.Flags().GetDuration("timeout")
		if err != nil {
			log.Fatal(err)
		}

		filename, err := cmd.Flags().GetString("config")
		if err != nil {
			log.Fatal(err)
@@ -263,7 +273,7 @@ func newInitCommand() *cobra.Command {

		config := fmt.Sprintf("instance %q {\n	access-token %q\n}\n", instance, token)

		c := createClientWithToken(baseURL, token)
		c := createClientWithToken(baseURL, token, timeout)
		user, err := metasrht.FetchMe(c.Client, ctx)
		if err != nil {
			log.Fatalf("failed to check OAuth2 token: %v", err)
diff --git a/main.go b/main.go
index cb548fa..398bb93 100644
--- a/main.go
+++ b/main.go
@@ -36,6 +36,7 @@ func main() {
	cmd.PersistentFlags().String("instance", "", "sr.ht instance to use")
	cmd.RegisterFlagCompletionFunc("instance", cobra.NoFileCompletions)
	cmd.PersistentFlags().String("config", "", "config file to use")
	cmd.PersistentFlags().Duration("timeout", 30*time.Second, "timeout to use")

	cmd.AddCommand(newBuildsCommand())
	cmd.AddCommand(newExportCommand())
-- 
2.36.1
Details
Message ID
<3JNzmIMfnnIA2cTXpbkaMFRkxng1BQv1Luatof43V25F4MpgSFS_k5E9zFm7WPdpnR0DasSRPKgS3dAr1w5iodrZTsc8UvEifatPy6ImF6Y=@emersion.fr>
In-Reply-To
<20220516150540.42563-1-blallo@autistici.org> (view parent)
DKIM signature
pass
Download raw message
On Monday, May 16th, 2022 at 17:02, Blallo <blallo@autistici.org> wrote:

> The http client used for interacting with the hut instance had a
> hardcoded 30 seconds timeout. While this might be ok in most cases, I
> happened to hit one of the corner cases when trying to push a tarball to
> site.ht. This commit adds a global flag to set the value, defaulting to
> the old one.

Hm. If possible, I'd prefer to do the right thing by default and set a
longer timeout for queries involving large file uploads, without
requiring the user to manually pass a --timeout option.
Details
Message ID
<20220626081614.q6basbarqr2qpjxf@rita>
In-Reply-To
<3JNzmIMfnnIA2cTXpbkaMFRkxng1BQv1Luatof43V25F4MpgSFS_k5E9zFm7WPdpnR0DasSRPKgS3dAr1w5iodrZTsc8UvEifatPy6ImF6Y=@emersion.fr> (view parent)
DKIM signature
pass
Download raw message
Simon Ser:
>On Monday, May 16th, 2022 at 17:02, Blallo <blallo@autistici.org> wrote:
>
>> The http client used for interacting with the hut instance had a
>> hardcoded 30 seconds timeout. While this might be ok in most cases, I
>> happened to hit one of the corner cases when trying to push a tarball to
>> site.ht. This commit adds a global flag to set the value, defaulting to
>> the old one.
>
>Hm. If possible, I'd prefer to do the right thing by default and set a
>longer timeout for queries involving large file uploads, without
>requiring the user to manually pass a --timeout option.

(Sorry for the late reply). I understand that it would be a great
improvement from the user experience perspective, but it is a bit
cumbersome to implement. I have two approaches in mind:
  - Let the client compute the size of the objects it wants to push to
    the remote
  - Let the client use retries with "exponential" timeout increase
    (like, first timeout 30s, then 60s, then a last try with 120s?)

Both approaches have drawbacks: in the first case either we have to
specify a body size (that we have to compute) for each call or consume
the io.Reader passed to the client **before** submitting the true
request; in the second case, the worst case has the user waiting for
30+60+120 = 210 seconds.

I prefer the first approach, but I want your opinion before trying to
do that.


Greetings

--
Blallo
Details
Message ID
<NWPVz4QyKgPHOFwL0brUZ-8vFsObnQKDMplMRCbvezIB1NXFeYvElZpkRmV30y-UurEMrA6lBLjXX6wDew_POxGKluAuBTVU8lwHh4k9vOQ=@emersion.fr>
In-Reply-To
<20220626081614.q6basbarqr2qpjxf@rita> (view parent)
DKIM signature
pass
Download raw message
On Sunday, June 26th, 2022 at 10:16, <blallo@autistici.org> wrote:

> Simon Ser:
>
> > On Monday, May 16th, 2022 at 17:02, Blallo blallo@autistici.org wrote:
> >
> > > The http client used for interacting with the hut instance had a
> > > hardcoded 30 seconds timeout. While this might be ok in most cases, I
> > > happened to hit one of the corner cases when trying to push a tarball to
> > > site.ht. This commit adds a global flag to set the value, defaulting to
> > > the old one.
> >
> > Hm. If possible, I'd prefer to do the right thing by default and set a
> > longer timeout for queries involving large file uploads, without
> > requiring the user to manually pass a --timeout option.
>
> (Sorry for the late reply). I understand that it would be a great
> improvement from the user experience perspective, but it is a bit
> cumbersome to implement. I have two approaches in mind:
> - Let the client compute the size of the objects it wants to push to
> the remote

This input file may come from stdin, in which case it's not possible to
compute the size.

> - Let the client use retries with "exponential" timeout increase
> (like, first timeout 30s, then 60s, then a last try with 120s?)
>
> Both approaches have drawbacks: in the first case either we have to
> specify a body size (that we have to compute) for each call or consume
> the io.Reader passed to the client before submitting the true
> request; in the second case, the worst case has the user waiting for
> 30+60+120 = 210 seconds.
>
> I prefer the first approach, but I want your opinion before trying to
> do that.

Can't we just use a fixed timeout of e.g. 5min, just for this command?
Reply to thread Export thread (mbox)