Issue HTTP calls in CacheBuster concurrently

[rhymes]

Is your feature request related to a problem? Please describe.

CacheBuster is one of the classes we use the most in our workflow but all of its HTTP calls are done sequentially.

For example, bust_comment sends around 34 HTTP calls to the upstream server - see #4744 (comment) - when called.

HTTP calls that have no dependency from one another.

Describe the solution you'd like

We should issue these calls concurrently to drastically lower the time taken for these methods to finish.

Isolated networking calls like these are the perfect example of concurrent programming.

I don't think we even use HTTP pipelining, which means that we open and close the TCP socket for each of the 34 calls.

We can use multi threading for this, as threads don't have to share data.

Describe alternatives you've considered

Async programming, but I'm not sure it can be easily done with Rails.

[citizen428]

I think overall this is a good idea, but I'm always a bit suspicious when someone wants to use threads in a Rails app, even if they don't share data. That's why I generally go for concurrent-ruby in cases like this. It offers many different patterns, is thread safe and receives more scrutiny and testing than anything we'd roll ourselves.

[citizen428]

In the meantime, a potential quick win could be to switch CacheBuster form HTTParty to http.rb (IMHO a nicer alternative) and use its persistent method so we at least don't repeatedly establish the HTTP connection:

HTTP.persistent("https://example.com") do |http|
  http.get("/").to_s # connect + write + read
  http.get("/").to_s # write + read
  http.get("/").to_s # write + read
end                  # close

[rhymes]

Yeah, usingThread.new manually wasn't in my plans anyway :D

Agreed also on switching to a library that uses HTTP pipelining.

I don't think it's urgent but it's something to think about. Hopefully we can decrease the number of HTTP calls also, now that we're slowly reviewing our Fastly caching structure

[cmgorton]

@rhymes do you think this should be labelled as. potential RFC?

[rhymes]

Hi @cmgorton, I think we should close this at the present moment.

I had a recent conversation with @atsmith813 and @vaidehijoshi and the way the caching code is evolving makes this less needed or potentially counter productive. It's still possible to achieve some paralellism but it might be more convienent to be done upstream (inside the caching server) as we're potentially switching to key/tag based caching instead of URL based caching.

All in all, if we circle back on the idea of having the client issue concurrent requests, we'll write down an RFC and link this issue for posterity.

Thanks for the reminder!