Deduplicating parellel requests

[ngehrsitz]

Hi,

I have the use case that an application is using multiple goroutines to make parallel requests that all try to access the same resource on a very slow webserver. With the current implementation of the cache all goroutines make their requests at the same time, thus the cache is still empty and each request will go through to the server. The server then takes several hundred milliseconds to respond and each of the responses then overwrites the value in the cache. This initial surge of duplicate requests is not ideal, but for most use cases the next round of requests would hit the cache and thus not face the same issue.
But the slow webserver I am accessing is serving realtime measurements from my battery storage system, thus caching these values is only useful for very short durations.
I see two options to fix this issue:

1. Rewrite the application so that the different goroutines do not make the same requests in parallel. Unfortunately this is not trivial since I am not the maintainer and the current logic would need to be severely rewritten to prevent these duplicate requests
2. Extend this cache so that it uses a https://pkg.go.dev/golang.org/x/sync/singleflight#Group when making requests so that parallel requests for the same resources are only fetched once.

I think that adding deduplication of parallel requests to the cache is probably the better idea since this will benefit all users of the cache and rewriting the logic to prevent these duplicate requests won´t be possible in every use case.
When looking at the code I am not sure where to best put this logic because we need to pass through cacheable and cacheKey from

httpcache/httpcache.go

Lines 767 to 769 in d376920

	func (t *Transport) RoundTrip(req *http.Request) (resp *http.Response, err error) {
	cacheKey := cacheKeyWithHeaders(req, t.CacheKeyHeaders)
	cacheable := (req.Method == methodGET || req.Method == methodHEAD) && req.Header.Get("range") == ""

to

httpcache/httpcache.go

Line 526 in d376920

func performRequest(transport http.RoundTripper, req *http.Request, onlyIfCached bool) (*http.Response, error) {

which then also needs to be a method of t *Transport. Do you have a better idea of how to implement this?

[sandrolain]

Hi @ngehrsitz,

First off, sorry for the delayed response - I didn't get a notification for this issue!
Thanks for the detailed explanation of your use case with the battery storage system.

Your idea is spot on! Using singleflight is exactly the right approach for your problem. I'll implement it.

Implementation

I'm planning to use golang.org/x/sync/singleflight - it's the standard Go extended library that's battle-tested in Kubernetes and Prometheus. There's also github.com/janos/singleflight which has some nice features like generics and better context cancellation, but I think the official one is a safer bet for now (we can always switch later if needed).

The implementation would be pretty straightforward - add an EnableSingleflight flag to the Transport struct (opt-in to keep backward compatibility) and wrap the request execution in singleflight.Do() using the cache key. This way when 100 goroutines hit your slow battery API at the same time, only 1 request actually goes through and everyone else gets the same response.

Quick Question

I'm currently working on v2 which has some breaking changes. Would it be okay to include this feature in v2, or do you need it in the current version?

If you're okay with v2, I can make it cleaner since I won't have to worry as much about backward compatibility. Otherwise, I can backport it to v1.x as an opt-in feature.

Let me know what works better for your use case!

[ngehrsitz]

I think integrating this into a v2 is the better approach. Getting such a version upgrade accepted into my project should not be an issue.
I already made an attempt to use implement a singleflight based solution and stumbled over the following hurdles:

1. You need to get cacheKey and cacheable into performRequest which means you end up with even more function parameters. My first approach here was to reduce the number of the parameters: ed607b2. But thinking about it again it might be a better approach to create a per-request based struct to carry that information.
2. The bigger issue I encountered was about sharing the body of the response:

func (t *Transport) performCacheableRequestOnce(req *http.Request, cacheable bool, cacheKey string) (*http.Response, error) {
	if !cacheable {
		return t.performRequest(req)
	}

	res, err, shared := t.requests.Do(cacheKey, func() (interface{}, error) {
		return t.performRequest(req)
	})
	resp, typeCorrect := res.(*http.Response)
	if !typeCorrect {
		return nil, errors.New("internal error: unexpected response type from singleflight")
	}

	if !shared {
		return resp, err
	}
	// The issue here is that all goroutines now have the same copy of resp.Body
	// But it can only be read by one of the goroutines once
	return t.makeResponseReusable(resp), err
}

The main issue here is that with singleflight you get the same response in every goroutine. Since body is a io.ReadCloser you now need to have some sort of locking so that it is only read by one goroutine. But for performance reasons you want this to be laizily initialized so that this only happens once the value is actually read. My solution to achieve this was to use sync.Once functions: ngehrsitz#1
Feel free to incorporate this into a v2.

[sandrolain]

You're absolutely right about the response body sharing problem - I completely missed that.

I'll integrate your implementation into v2.

Thanks for not just identifying the issue but also implementing and testing a working solution - really appreciate it!

[sandrolain]

Hi @ngehrsitz, in the develop branch you can find the almost complete version of v2, including the introduction of deduplication and other optimizations.