context is not respected for HTTP requests

[zeisss]

It seems the passed Ctx to getter.Client is not passed to the http.Request:

https://github.com/hashicorp/go-getter/blob/main/get_http.go#L85-L94

I wrote a test for this below. I receive the context.Canceled error, but it still waits for both requests (HEAD + GET) to finish, thus this test takes 6 seconds instead of <1sec.

package cache

import (
	"context"
	"errors"
	"net/http"
	"net/http/httptest"
	"testing"
	"time"

	"github.com/hashicorp/go-getter"
)

func TestHashicorpGoGetter__RespectsContextCanceled(t *testing.T) {
	ctx, cancel := context.WithCancel(context.Background())
	cancel() // cancel immediately

	srv := GivenHTTPServer(t, http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
		t.Logf("Received %s %s", req.Method, req.URL)
		time.Sleep(3 * time.Second)
	}))

	tmpdir := t.TempDir()
	g := getter.Client{
		Ctx:  ctx,
		Src:  srv.URL,
		Pwd:  tmpdir,
		Dst:  "testfile",
		Mode: getter.ClientModeFile,
	}

	t1 := time.Now()
	err := g.Get()
	dur := time.Since(t1)

	if !errors.Is(err, context.Canceled) {
		t.Log("error is not context.Canceled")
		t.Fail()
	}
	if dur > 1*time.Second {
		t.Log("Get() took too long")
		t.Fail()
	}
}

func GivenHTTPServer(t testing.TB, h http.Handler) *httptest.Server {
	srv := httptest.NewServer(h)
	t.Cleanup(func() {
		srv.CloseClientConnections()
		srv.Close()
	})
	return srv
}

Obviously it is more realistic to cancel the context during the request. This could be achieved by moving the cancel() call into the http handler.

[azr]

Hello @zeisss, thanks for opening, do you see the same issue with the v2 branch ?

[zeisss]

I am not sure how to test that, since v2 seems to be weird with regards to go get (maybe I am misunderstanding go get wrong):

% go get github.com/hashicorp/go-getter@v2
go: errors parsing go.mod:
/Users/zeisss/p/stormforger/loadgenerator/go.mod:22:2: no matching versions for query "v2"

But looking at the code at https://github.com/hashicorp/go-getter/blob/v2/get_http.go#L83-L91 I believe the issue is still around. I would expect a call to http.NewRequestWithContext(), but I seehttp.NewRequest().

I can double check my suspicion for v2 if you require it.

[azr]

Hey @zeisss, thanks for taking a look, I agree with you conclusion, would you like to open a PR to fix that ? Thanks !

[zeisss]

@azr for completness sake: Do you want me to make a similar PR for your v2 branch or are the maintainers merging this?

[azr]

Hey @zeisss, thanks for asking, if you have the time for making one in the v2 I'd love to have this there too ! 🙂

[zeisss]

I created #324 with a fix for the v2 branch.

[zeisss]

I hope its ok to keep this ticket busy a bit longer: Are there any plans to make a release soonish or should I update my project to use main?

[azr]

v1.5.4 was just released !

[zeisss]

I noticed! Nice and thanks for you help!

Definately closed now :)