Add keep-alive support #171

Open
wants to merge 2 commits into
from

Projects

None yet
@byroot

I simply use net/http/persistent for easy keep-alive implementation

A little benchmark:

require './rest-client'
require 'benchmark'

puts Benchmark.measure{ 10.times{ RestClient.get('http://www.google.com/').length } }
RestClient.persistent = true
puts Benchmark.measure{ 10.times{ RestClient.get('http://www.google.com/').length } }

I get those numbers

  0.080000   0.010000   0.090000 (  1.365598)
  0.030000   0.000000   0.030000 (  0.354387)

For now the integration is a bit "Quick & Dirty". It's just to show the eventual benefits.
But if you like the idea I can clean it up and add test coverage.

Regards,

@L2G
REST-Client Team member

I dig this. I haven't looked at it yet, but I can't believe no one's done it yet. 😸

@byroot

Probably because of #44. Somebody said it's not thread-safe which is false.

@L2G
REST-Client Team member

Or, since that was a concern raised a couple of years ago, it may have been true for Ruby 1.8.x but no longer true for >= 1.9.1? (I'm not concerned with keeping compatible with 1.8.x as I believe it's being "sunsetted" soon.)

@byroot

Anyway you don't have to worry because it's thread-safe from day one: https://github.com/drbrain/net-http-persistent/tree/v1.0

Their statement is right, reusing the same connection prevent thread safety, unless you keep one connection per thread, and this it what net/http/persistent do.

@L2G
REST-Client Team member

Ahh, okay.

@byroot

IHMO the question is more: should it be implemented as an option, or should we always use persistent connections if available.

I'm more for the second one, because it lead to a simpler code and IHMO persistent connections are preferable in most real world cases.

But I'm no maintainer.

@L2G
REST-Client Team member

I think using persistent connections when available is a reasonable default, but then there should be an option to turn it off in case of unforeseen situations where it causes problems or just doesn't work as it should.

@byroot

Ok then, I'll try to update this PR soon.

@byroot

I've refactored the code, and updated the test suite.
Now all connections are handled by Net::HTTP::Persistent, but when asked to do so connections are automatically closed.
I also use two distinct pools of connection so you can use both at the same time.

Persistent connections are now the default, but can be turned off globaly with

RestClient.persistent = false

Or on a request basis by passing :persistent => false

If you are not confident with making persistent connections the default you can
easily revert it by removing the @persistent = true line in restclient.rb.
The test suite will pass whatever the default is.

Regards

@dukex

Somethings about it?

@mjackson

@L2G I'm with you -- can't believe this hasn't been done yet. :) Is there any hang up with this change? Or can it be applied as is?

@byroot byroot Add keep-alive support
For ease of implementation it rely on the excellent net-http-persistent.

Persistent connections are now the default, but can be turned off globaly with
```ruby
RestClient.persistent = false
```
Or on a request basis by passing `:persistent => false`

If you are not confident with making persistent connections the default you can
easily revert it by removing the `@persistent = true` line in `restclient.rb`.
The test suite will pass whatever the default is.
af5ceb0
@byroot

@mjijackson I've just rebased it. And yes it's ready to merge.

@byroot

Oh, and I use it in production since 3 months against Stripe API, it works like a charm and provide nice speed improvements.

@duncan-bayne

Please, could someone merge this? Without it, RestClient is useless for my needs :(

@ghost

Yeah, I could use the keep-alive option myself too with couchrest. Merging would be great!

@jesseangell

+1 -- this should be merged! It's extremely painful using RESTful services over SSL without persistent connections.

@tony-spataro-rs

+1 again for merging this pull request; I badly need persistent HTTP connections and I DON'T want to fork rest-client (or roll my own!)

@duncan-bayne

@tony-spataro-rs - I've forked rest-client from the pull request branch. In your Gemfile you can do:

gem 'rest-client', :github => 'duncan-bayne/rest-client', :branch => 'feature-keep-alive'

Although note that I haven't been maintaining it in terms of merging back from this project. YMMV. No warranties. Santa might eat your puppies. Etc.

It does work well enough for me though.

@tony-spataro-rs
@ab
REST-Client Team member
ab commented Mar 17, 2014

Hi all, I'm looking to get this merged soon. But I don't think we should make it default in a minor release. There are a number of areas where Net::HTTP::Persistent provides a different API from Net::HTTP in ways that could be very surprising: in the SSL behavior, in the exceptions raised (it rescues things like Errno::ECONNREFUSED and raises Net::HTTP::Persistent::Error instead).

@byroot

@ab no problem, you just need to remove this line: https://github.com/rest-client/rest-client/pull/171/files#diff-acdc459e7a74c777e18f314848bc93b4R101

Do you need a rebase?

@ab
REST-Client Team member
ab commented Mar 17, 2014

Got it, thanks! There are going to be a bunch of other merge conflicts with the SSL stuff I'm working on, so I can take care of doing the rebase.

@mikhailov

So it looks like persistent becoms default, has it merged in stable branch yet?

@ab ab modified the milestone: 1.8.0, 1.7.0 Jul 8, 2014
@cacofonix

@ab Any particular reason you held off on merging this? Alternative solution?

@ab ab removed the ready label Mar 24, 2015
@ab
REST-Client Team member
ab commented Mar 24, 2015

This is still on my mind, but I'm not yet going to be able to get to this in the next release. The blocker is that it introduces major inconsistency in the exceptions raised / API / expected behavior between when persistence is enabled vs. not.

Ideally we would implement keepalive support in a way that does not change the API.

@ab ab modified the milestone: 2.1.x, 1.8.0 Mar 24, 2015
@byroot

Since ruby 2.0, the net/http in the stdlib support keep alive out of the box. You simply need to set the keep_alive_timeout property.

I no longer use rest-client, so I won't update this PR. But you guys should be able to reimplement this easily without any drawback.

@ab ab self-assigned this Apr 27, 2015
@atz atz referenced this pull request in sul-dlss/argo Nov 25, 2015
Closed

Analysis of profiling data #250

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment