Skip to content

Commit

Permalink
Cache Faraday::Connection for persistent adapters
Browse files Browse the repository at this point in the history
In order to be a good web citizen and to allow maximum performance HTTP
clients should use persistent connections to make requests.  This has
multiple benefits:

* Users won't be delayed by TCP slow-start figuring out the window size
  for every HTTP request
* Users won't be delayed by TLS handshakes for each new request
* Bandwidth overhead per request is reduced through large TCP windows on
  established TLS connections.

Some Slack endpoints are paginated and may take many requests to fetch
all necessary data.  Using persistent connections can result in a 4x
speed boost.

Slack::Web::Client uses Faraday which uses Net::HTTP by default.  While
Net::HTTP is capable of persistent connections Net::HTTP + Faraday is
not configured this way.  The Faraday::Adapter::NetHttpPersistent
adapter does.

Slack::Web::Client uses Faraday#default_adapter so the end user can
switch adapters are used like this:

    Faraday.default_adapter = Faraday::Adapter::NetHttpPersistent

    c = Slack::Web::Client.new …

Unfortunately Slack::Web::Client does not cache the Faraday::Connection
object and instead creates a new connection for every request.  This
breaks the ability to use persistent connections with
Slack::Web::Client.

This can be observed through using Wireshark with an `ssl.handshake`
filter, or by observing `SYN` packets sent by `tcpdump host slack.com`.

This patch adds caching of the Faraday::Connection object to
Slack::Web::Client to allow caching of connections through Faraday.  A
cached connection can be reused as a persistent connection.

Recommending users use a persistent connection adapter (like
Faraday::Adapter::NetHttpPersistent) is not part of this pull request,
but I do recommend it.  Your users should see an easy speed improvement.

Here is a `time` output from a script that fetches ~4,000 conversations
using the net-http-persistent adapter without this patch (making it
ineffective):

    $ time ruby t.rb
    ruby t.rb  2.46s user 0.25s system 14% cpu 18.389 total

and with this patch:

    $ time ruby t.rb
    ruby t.rb  0.99s user 0.20s system 13% cpu 9.053 total

(This is only a 2x speed boost as the slack `conversation.list` API is
highly variable in response time.)
  • Loading branch information
drbrain authored and dblock committed May 8, 2020
1 parent 6f49e2b commit 225c71e
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 20 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
### 0.14.7 (Next)

* [#322](https://github.com/slack-ruby/slack-ruby-client/pull/322): Cache `Faraday::Connection` for persistent adapters - [@drbrain](https://github.com/drbrain).
* Your contribution here.

### 0.14.6 (2020/3/28)
Expand Down
43 changes: 23 additions & 20 deletions lib/slack/web/faraday/connection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,29 +6,32 @@ module Connection
private

def connection
options = {
headers: { 'Accept' => 'application/json; charset=utf-8' }
}
@connection ||=
begin
options = {
headers: { 'Accept' => 'application/json; charset=utf-8' }
}

options[:headers]['User-Agent'] = user_agent if user_agent
options[:proxy] = proxy if proxy
options[:ssl] = { ca_path: ca_path, ca_file: ca_file } if ca_path || ca_file
options[:headers]['User-Agent'] = user_agent if user_agent
options[:proxy] = proxy if proxy
options[:ssl] = { ca_path: ca_path, ca_file: ca_file } if ca_path || ca_file

request_options = {}
request_options[:timeout] = timeout if timeout
request_options[:open_timeout] = open_timeout if open_timeout
options[:request] = request_options if request_options.any?
request_options = {}
request_options[:timeout] = timeout if timeout
request_options[:open_timeout] = open_timeout if open_timeout
options[:request] = request_options if request_options.any?

::Faraday::Connection.new(endpoint, options) do |connection|
connection.use ::Faraday::Request::Multipart
connection.use ::Faraday::Request::UrlEncoded
connection.use ::Faraday::Response::RaiseError
connection.use ::Slack::Web::Faraday::Response::RaiseError
connection.use ::FaradayMiddleware::Mashify, mash_class: Slack::Messages::Message
connection.use ::FaradayMiddleware::ParseJson
connection.response :logger, logger if logger
connection.adapter ::Faraday.default_adapter
end
::Faraday::Connection.new(endpoint, options) do |connection|
connection.use ::Faraday::Request::Multipart
connection.use ::Faraday::Request::UrlEncoded
connection.use ::Faraday::Response::RaiseError
connection.use ::Slack::Web::Faraday::Response::RaiseError
connection.use ::FaradayMiddleware::Mashify, mash_class: Slack::Messages::Message
connection.use ::FaradayMiddleware::ParseJson
connection.response :logger, logger if logger
connection.adapter ::Faraday.default_adapter
end
end
end
end
end
Expand Down
11 changes: 11 additions & 0 deletions spec/slack/web/client_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -205,5 +205,16 @@
client.users_admin_setInactive(user: 'U092BDCLV')
end
end

context 'persistent capability' do
describe '#initialize' do
it 'caches the Faraday connection to allow persistent adapters' do
first = client.send(:connection)
second = client.send(:connection)

expect(first).to equal second
end
end
end
end
end

0 comments on commit 225c71e

Please sign in to comment.