Fix request-map not being cleared up after requests

[gregjones]

Fixes #62

I think this will do until sorting out the more up-to-date cancelling can happen.

cc @kegsay @shurcooL

[dmitshur]

See inline suggestion about improving code clarity.

Functionality-wise, LGTM.

[dmitshur]

What does the name of variable keyReq mean? Is it the "request that is the key of the modReq map"?

I think this code is quite subtle and not obvious, so an inline comment explaining the purpose of this line would be very helpful. Something like this:

keyReq := req // Keep a pointer to the original request so we can delete it from the `modReq` map later.

[dmitshur]

I've opened up the editor and played more with this code... I've realized this could be written slightly differently.

Right now, it sets the mapping, then sets req to req2, and finally tries to defer deleting the mapping if something went wrong.

Instead, how about rearranging that order so that first we set the mapping, then immediately defer deleting the mapping (it still runs later, when func returns), and only then do we override req with req2.

I think that'd be a lot easier to understand. It's akin to writing code like this:

f, err := f.Open(...)
if err != nil {
	return err
}
defer f.Close() // Defer right away.
// ... use f

So, my proposed alternative implementation can look like this:

if req2 != nil {
	// Associate original request with cloned request so we can refer to
	// it in CancelRequest(). Release the mapping after it's no longer needed.
	t.setModReq(req, req2)
	defer func(originalReq *http.Request) {
		// Release req/clone mapping on error
		if err != nil {
			t.setModReq(originalReq, nil)
		}
		if resp != nil {
			// Release req/clone mapping on body close/EOF
			resp.Body = &onEOFReader{
				rc: resp.Body,
				fn: func() { t.setModReq(originalReq, nil) },
			}
		}
	}(req)

	req = req2
}

What do you think @gregjones, is that more clear or not?

[dmitshur]

It's great to have a test for this bug. 👍

[gregjones]

I like your suggestion, thanks 👍

[kegsay]

Any ETA on this landing? My project's memory usage has doubled in size since Monday (~500MB -> ~1GB in under 4 days) so I really want to apply this fix as soon as possible 🙏

[dmitshur]

/cc @gregjones Are you waiting on anything before merging this? Your latest changes have my LGTM.