rubygems/rubygems — pull request #869 by tenderlove (2014-03-27 22:07:26+00:00)
Title: Reuse persistent connections to speed up Gem install

Reusing persistent connections gives us a nice speed boost. I'll start with benchmarks.

Before my patch:

```
[aaron@higgins rubygems (master)]$ time ruby -I lib bin/gem install rails --no-ri --no-rdoc
Successfully installed rails-4.0.4
1 gem installed

real	0m12.838s
user	0m1.115s
sys	0m0.309s
```

After my patch:

```
[aaron@higgins rubygems (persistent)]$ time ruby -I lib bin/gem install rails --no-ri --no-rdoc
Successfully installed rails-4.0.4
1 gem installed

real	0m8.134s
user	0m0.871s
sys	0m0.287s
```

This is on my Mifi, but I saw similar improvements at home.

First I will cover the problems with connection handling in RubyGems so that I can contrast the features of this pull request.

The [`connection_for` method in `Gem::Request`](https://github.com/rubygems/rubygems/blob/267d7048d3ffdf7db18885ae8414152e5bc9e050/lib/rubygems/request.rb#L73) hands out an http connection. The only place that `connection_for` is called in the RubyGems code base (besides the tests) is [the fetch method](https://github.com/rubygems/rubygems/blob/267d7048d3ffdf7db18885ae8414152e5bc9e050/lib/rubygems/request.rb#L122).

## Thread safety problems

If you read the [caching code in the method](https://github.com/rubygems/rubygems/blob/267d7048d3ffdf7db18885ae8414152e5bc9e050/lib/rubygems/request.rb#L85-L90) it's meant to look as if it is thread safe. However, you'll note that the only method that calls this method [is not thread safe](https://github.com/rubygems/rubygems/blob/267d7048d3ffdf7db18885ae8414152e5bc9e050/lib/rubygems/request.rb#L128).

## Reuse problems

### Reuse problems pertaining to thread safety

It turns out that it doesn't actually matter if the `Gem::Request` object is thread safe or not. This is because the only place where `Gem::Request` is used is [in the `request` method of `Gem::Fetcher`](https://github.com/rubygems/rubygems/blob/267d7048d3ffdf7db18885ae8414152e5bc9e050/lib/rubygems/remote_fetcher.rb#L336-342) and is immediately thrown away. There are no threads sharing this object, so it doesn't really matter if the object is thread safe or not.

### Reuse problems pertaining to the connection

As I mentioned earlier, the `Gem::Request` object is never reused. This renders [the connection cache in the object useless](https://github.com/rubygems/rubygems/blob/267d7048d3ffdf7db18885ae8414152e5bc9e050/lib/rubygems/request.rb#L87-L90). This also means that the [keep-alive headers set in the fetch method are also not necessary](https://github.com/rubygems/rubygems/blob/267d7048d3ffdf7db18885ae8414152e5bc9e050/lib/rubygems/request.rb#L112-L114). We effectively open a Keep-Alive connection for every request, then just don't close it off until the `Gem::Request` object is garbage collected (sorry Heroku!)

## This Pull Request

This pull request introduces a connection pool, [and reuses that pool between requests](https://github.com/tenderlove/rubygems/blob/1be410f5fa399b022f74756f87987387d6ef7a08/lib/rubygems/remote_fetcher.rb#L340-343). I've made the connection pool thread safe, so it can be shared between threads.

**This connection pool only maintains one connection per proxy, per server**. It's not clear to me that using multiple connections in separate threads would be a win since each would have to establish an SSL connection. You can clearly see above that just reusing one connection is a nice speed gain. We need to study how RubyGems uses connections and make a decision based on request usage (e.g. only use one connection for many small requests like bundler API, and maybe many connections for downloading gems). It could be that we only want to use one connection per proxy per server, and we could just rm most of this connection management code.

I realize this is a fairly large patch, so I've split it in to many commits. I am willing to submit each commit as a PR separately and explain them in order to get this merged in. The tests all pass on each commit.