fix: use proper HTTP clients to fetch files

[rebornplusplus]

Fixes #14.

As suggested below by Gustavo, use the short timeout httpClient and long timeout bulkClient in archive.go properly. Use the quick httpClient to fetch small files such as Release files. For packages and indexes, use the bulkClient as the files can be quite big.

---

• Have you signed the CLA?

[cjdcordeiro]

Nice, thanks. Just a couple of comments and a question: is it possible to have a test for this in archive_test?

[rebornplusplus]

Added a test for the new option. Please make sure to pass the --real-archive option to go test to run this test.

[cjdcordeiro]

Added a test for the new option. Please make sure to pass the --real-archive option to go test to run this test.

Cool!
then it should also be added to the CI test shouldn't it?

[rebornplusplus]

I was a bit hesitant to run the tests with --real-archive in the GitHub workflows test because it might simply raise an error because of a slow connection to the archives. But I guess that is for the better. If it times out or errors out here, it'd probably give us a good idea if the issue is still persistent.

Additionally, the new ./internal/archive test run with --real-archive does not contribute to the coverprofile test-coverage.out. Let me know if you want me to include those in there as well. In that case, I would probably need to use the gocovmerge tool to merge two coverprofiles.

[woky]

I'd prefer the timeout to be passed down via archive.Options but that may be tricky as in the Archive interface we don't use http.Client directly but rather just the global httpDo() function. Apparently it's possible to create interruptible requests via NewRequestWithContext() but that would need to be cancelled from another goroutine.

In conclusion, LGTM. :-)

[cjdcordeiro]

ty!

[niemeyer]

There's a detail I mention below about the option name, but reviewing this code made me realize that the implementation is currently misusing a concept that I've ported from earlier applications that I wrote a while ago. Note how we have two HTTP clients: httpCilent, and bulkClient, with different timeouts explicitly defined. The distinction exists precisely because one of them provides interactivity for smaller API-like content, while the latter provides a configuration for bulk data (IOW, large downloads, such as packages and Content files).

So this change is in effect working around the fact we're misusing these two values: it's strugling with the client made for quick interactions, and attempting to turn it into the bluk client. We might as well actually use the two clients properly instead.

So here is the proposal: can we please drop the explicit flag for now, and make this PR simply change the implementation to use the two different clients correctly, for the proper cases?

We can do that by simply introducing a flag parameter to the fetch() function:

type fetchFlags uint

const (
        fetchBulk    fetchFlags = 1 << iota
        fetchDefault fetchFlags = 0
)

func (a ...) fetch(..., flags fetchFlags) ... { ... }

Then, packages can use the bulk client, while indexes can use the short timeout one.

How does that sound?

[rebornplusplus]

Hiya @niemeyer, thanks for the insight! I also wondered what the intentions of bulkClient was while inspecting the source for this PR. I have dropped the previous changes and used the HTTP clients as you suggested.

Although you suggested using the shorter timeout httpClient for indexes, I am using the bulkClient. The reason for that is one of the index files, namely the one for jammy suite's universe component, is quite big; a whopping 17M. Please see Packages.gz at http://archive.ubuntu.com/ubuntu/dists/jammy/universe/binary-amd64/. As shown in #14 and as I elaborated in the Jira ticket, this file is one of the main reasons for Chisel to fail with a context deadline exceeded error.

Perhaps, using the bulk client for downloading indexes doesn't seem intuitive or natural. Let me know if you want to use the shorter timeout httpClient for downloading indexes anyway. In that case, I would encourage to increase the timeout for the client from 30s to 60s. Asking to download 17M in 30s seems a bit unfair.

[niemeyer]

Thanks for the changes.