Add EM-HttpRequest adapter #40

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
@RISCfuture
Contributor

RISCfuture commented Jun 28, 2011

At my company (recurly.com) we use Active Merchant to communicate with a variety of different payment gateways. Normally Active Merchant just uses Net::HTTP, but since we're using Thin, we wanted to take advantage of fibers and use EventMachine plus EM-HttpRequest.

To do this, I added an em-http-request adapter to HTTPI, and created an ActiveMerchant::Billing::Net::HTTP object that responds to the same interface as ::Net::HTTP except it just hands the request off to HTTPI (clever, eh?). That way, we can use the :em_http adapter for most gateways, and if a particular gateway requires a feature not supported by EM-HttpRequest, we can fall back on the :net_http adapter.

In addition, we can use evented I/O over Savon to communicate with our vault.

@jots

This comment has been minimized.

Show comment
Hide comment
@jots

jots Sep 15, 2011

This is useful to me. Thanks for writing it. Will it be merged in?

jots commented Sep 15, 2011

This is useful to me. Thanks for writing it. Will it be merged in?

@rbriank

This comment has been minimized.

Show comment
Hide comment
@rbriank

rbriank Dec 8, 2011

I made use of this also -- please merge it!

rbriank commented Dec 8, 2011

I made use of this also -- please merge it!

@ebeigarts

This comment has been minimized.

Show comment
Hide comment
@ebeigarts

ebeigarts Dec 9, 2011

Contributor

+1

Contributor

ebeigarts commented Dec 9, 2011

+1

+
+ def _request(request)
+ options = client_options(request)
+ client = EventMachine::HttpRequest.new("#{request.url.scheme}://#{request.url.host}:#{request.url.port}#{request.url.path}")

This comment has been minimized.

@rbriank

rbriank Dec 9, 2011

I'm using this gem in ruby 1.9.3 and when this called, it was ignoring the options.

I added options:

client = EventMachine::HttpRequest.new("#{request.url.scheme}://#{request.url.host}:#{request.url.port}#{request.url.path}", options)

Has yield changed in 1.9.3 so the options are not passed? I only noticed because I was trying to set the timeouts, and they were using defaults regardless of what I set.

@rbriank

rbriank Dec 9, 2011

I'm using this gem in ruby 1.9.3 and when this called, it was ignoring the options.

I added options:

client = EventMachine::HttpRequest.new("#{request.url.scheme}://#{request.url.host}:#{request.url.port}#{request.url.path}", options)

Has yield changed in 1.9.3 so the options are not passed? I only noticed because I was trying to set the timeouts, and they were using defaults regardless of what I set.

This comment has been minimized.

@RISCfuture

RISCfuture Dec 9, 2011

Contributor

We haven't upgraded to the latest EventMachine here yet, but from what I understand, a lot of options that used to be on the client are now on the request (or vice versa), so it's possible the gem needs to be updated to pass the correct options to the correct methods #new vs. #request.

@RISCfuture

RISCfuture Dec 9, 2011

Contributor

We haven't upgraded to the latest EventMachine here yet, but from what I understand, a lot of options that used to be on the client are now on the request (or vice versa), so it's possible the gem needs to be updated to pass the correct options to the correct methods #new vs. #request.

This comment has been minimized.

@rubiii

rubiii Dec 13, 2011

Contributor

i unfortunately don't know about the current state of eventmachine, but if you guys can let me know whether this pull request is still up to date, i'll gladly merge it in and release a new version asap.

@rubiii

rubiii Dec 13, 2011

Contributor

i unfortunately don't know about the current state of eventmachine, but if you guys can let me know whether this pull request is still up to date, i'll gladly merge it in and release a new version asap.

This comment has been minimized.

@rbriank

rbriank Dec 17, 2011

It would also need to be upgraded to 0.9.5 which has some configuration changes.

@rbriank

rbriank Dec 17, 2011

It would also need to be upgraded to 0.9.5 which has some configuration changes.

@tomas-stefano

This comment has been minimized.

Show comment
Hide comment
@tomas-stefano

tomas-stefano Feb 22, 2012

Awesome! This is very useful to me. Will it be merged in?

Awesome! This is very useful to me. Will it be merged in?

@rubiii

This comment has been minimized.

Show comment
Hide comment
@rubiii

rubiii Feb 22, 2012

Contributor

if anyone can update this to up-to-date versions of eventmachine and dependencies, i'll make sure to merge it.

Contributor

rubiii commented Feb 22, 2012

if anyone can update this to up-to-date versions of eventmachine and dependencies, i'll make sure to merge it.

@rbriank

This comment has been minimized.

Show comment
Hide comment
@rbriank

rbriank Feb 23, 2012

I'll see what I can do over the weekend.

rbriank commented Feb 23, 2012

I'll see what I can do over the weekend.

@canbuffi

This comment has been minimized.

Show comment
Hide comment
@canbuffi

canbuffi Mar 5, 2012

+1

A merge into the official gem would be fantastic. I'd like to use evented Savon as well.

canbuffi commented Mar 5, 2012

+1

A merge into the official gem would be fantastic. I'd like to use evented Savon as well.

@zond

This comment has been minimized.

Show comment
Hide comment
@zond

zond Mar 12, 2012

We are using this right now, with a recent version of eventmachine, and it works just fine.
Could someone list the issues blocking this from merge into master? It would be great to get it done and use a vanilla rubyforge version in our environment :)

zond commented Mar 12, 2012

We are using this right now, with a recent version of eventmachine, and it works just fine.
Could someone list the issues blocking this from merge into master? It would be great to get it done and use a vanilla rubyforge version in our environment :)

@lukebyrne

This comment has been minimized.

Show comment
Hide comment
@lukebyrne

lukebyrne May 11, 2012

Hi All,

Just wondering where this is at. I was wanting to use this in my gem github.com/lukebyrne/betfair.

I want to be able to add a simple flag when the class is instantiated to switch to using em-http, in much the same way you can specify gzip => true

Would someone please spell out to a simple man how I would get the forked version of httpi to override the bundled one in savon, and then to setup savon to use em-http?

Cheers,

LB

Hi All,

Just wondering where this is at. I was wanting to use this in my gem github.com/lukebyrne/betfair.

I want to be able to add a simple flag when the class is instantiated to switch to using em-http, in much the same way you can specify gzip => true

Would someone please spell out to a simple man how I would get the forked version of httpi to override the bundled one in savon, and then to setup savon to use em-http?

Cheers,

LB

@zond

This comment has been minimized.

Show comment
Hide comment
@zond

zond May 11, 2012

We used a :git entry in our Gemfile, and then used bundler to install everything.

Bundler won't install the vanilla httpi gem if you have already installed it from another source.

zond commented May 11, 2012

We used a :git entry in our Gemfile, and then used bundler to install everything.

Bundler won't install the vanilla httpi gem if you have already installed it from another source.

@rubiii

This comment has been minimized.

Show comment
Hide comment
@rubiii

rubiii May 11, 2012

Contributor

i merged the changes, fixed a few things and pushed an em_http adapter branch which unfortunately fails the specs
for 1.9 and jruby. is anyone able to take a look at it?

Contributor

rubiii commented May 11, 2012

i merged the changes, fixed a few things and pushed an em_http adapter branch which unfortunately fails the specs
for 1.9 and jruby. is anyone able to take a look at it?

@rubiii rubiii referenced this pull request in savonrb/savon May 11, 2012

Closed

Any chance of Event Machine intergration? #286

@lukebyrne

This comment has been minimized.

Show comment
Hide comment
@lukebyrne

lukebyrne May 14, 2012

zond - and to need you need to specify that you want to explicitly use em-http when setting up a savon connection. I had a look and it seems that it will try it last or third. How do you change this order?

zond - and to need you need to specify that you want to explicitly use em-http when setting up a savon connection. I had a look and it seems that it will try it last or third. How do you change this order?

@zond

This comment has been minimized.

Show comment
Hide comment
@zond

zond May 14, 2012

lukebyrne:

You only need to tell HTTPI to use em-http, Savon will use whatever HTTPI is configured to use iirc. We did:

HTTPI::Adapter.use = :em_http

zond commented May 14, 2012

lukebyrne:

You only need to tell HTTPI to use em-http, Savon will use whatever HTTPI is configured to use iirc. We did:

HTTPI::Adapter.use = :em_http
@turboladen

This comment has been minimized.

Show comment
Hide comment
@turboladen

turboladen Jul 1, 2012

Contributor

+1

Contributor

turboladen commented Jul 1, 2012

+1

@turboladen

This comment has been minimized.

Show comment
Hide comment
@turboladen

turboladen Jul 1, 2012

Contributor

@rubiii I pulled down the em_http adapter branch and ran the tests on 1.9.3 and they pass for me... Having problems with them running on JRuby 1.6.7 at the moment though (same em-synchrony problem that the travis log shows). I noticed, however, in the travis logs that bundler pulled down eventmachine 1.0.0.beta.4; 1.0.0.rc.4 is out now, so I wonder if the failure you encountered was related to that? I'll let you know if I can figure anything out with the jruby thing...

Contributor

turboladen commented Jul 1, 2012

@rubiii I pulled down the em_http adapter branch and ran the tests on 1.9.3 and they pass for me... Having problems with them running on JRuby 1.6.7 at the moment though (same em-synchrony problem that the travis log shows). I noticed, however, in the travis logs that bundler pulled down eventmachine 1.0.0.beta.4; 1.0.0.rc.4 is out now, so I wonder if the failure you encountered was related to that? I'll let you know if I can figure anything out with the jruby thing...

@turboladen

This comment has been minimized.

Show comment
Hide comment
@turboladen

turboladen Jul 2, 2012

Contributor

@rubiii I got the JRuby tests to pass locally by making the Gemfile bundle for :java. The em gems weren't getting bundled when tested under jruby because of this, and thus couldn't be required. I also had to change this for curb as well, since all of those specs failed once I got passed the em stuff.

Changing this got them to pass for me on MRI 1.9 and JRuby:

gem "curb",            "~> 0.7.8", :require => false, :platforms => [:ruby, :jruby]
gem 'em-http-request',             :require => false, :platforms => [:ruby, :jruby]
gem 'em-synchrony',                :require => false, :platforms => [:ruby, :jruby]
Contributor

turboladen commented Jul 2, 2012

@rubiii I got the JRuby tests to pass locally by making the Gemfile bundle for :java. The em gems weren't getting bundled when tested under jruby because of this, and thus couldn't be required. I also had to change this for curb as well, since all of those specs failed once I got passed the em stuff.

Changing this got them to pass for me on MRI 1.9 and JRuby:

gem "curb",            "~> 0.7.8", :require => false, :platforms => [:ruby, :jruby]
gem 'em-http-request',             :require => false, :platforms => [:ruby, :jruby]
gem 'em-synchrony',                :require => false, :platforms => [:ruby, :jruby]

rubiii added a commit that referenced this pull request Jul 2, 2012

@rubiii

This comment has been minimized.

Show comment
Hide comment
@rubiii

rubiii Jul 2, 2012

Contributor

@turboladen thanks for following up on this. i applied your change and it solved the problem with jruby for me.
but travis is still complaining: http://travis-ci.org/#!/rubiii/httpi/jobs/1755578

for 1.9.x the integration specs fail with a runtime error. i can reproduce the problem locally, but would need to investigate. and jruby has problems with curb.

you can ignore the rbx errors for now. they're related to a problem with gzip.

Contributor

rubiii commented Jul 2, 2012

@turboladen thanks for following up on this. i applied your change and it solved the problem with jruby for me.
but travis is still complaining: http://travis-ci.org/#!/rubiii/httpi/jobs/1755578

for 1.9.x the integration specs fail with a runtime error. i can reproduce the problem locally, but would need to investigate. and jruby has problems with curb.

you can ignore the rbx errors for now. they're related to a problem with gzip.

@rubiii

This comment has been minimized.

Show comment
Hide comment
@rubiii

rubiii Jul 2, 2012

Contributor

travis doesn't support c extensions, so i excluded the specs for curb on jruby.
mri and jruby 1.9 still don't pass with event machine: http://travis-ci.org/#!/rubiii/httpi/builds/1756232

Contributor

rubiii commented Jul 2, 2012

travis doesn't support c extensions, so i excluded the specs for curb on jruby.
mri and jruby 1.9 still don't pass with event machine: http://travis-ci.org/#!/rubiii/httpi/builds/1756232

@turboladen

This comment has been minimized.

Show comment
Hide comment
@turboladen

turboladen Jul 10, 2012

Contributor

Sorry I went dark on you... I plan on looking into these soon. If I get it working, I'll just submit a pull request.

Contributor

turboladen commented Jul 10, 2012

Sorry I went dark on you... I plan on looking into these soon. If I get it working, I'll just submit a pull request.

@jonhyman

This comment has been minimized.

Show comment
Hide comment
@jonhyman

jonhyman Aug 27, 2012

@zond, after updating the adapter to :em_http, how do you access the callback/errback for a request?

@zond, after updating the adapter to :em_http, how do you access the callback/errback for a request?

@zond

This comment has been minimized.

Show comment
Hide comment
@zond

zond Aug 27, 2012

@jonhyman, I don't, we only used this to enable EM connections in Savon so as not to block our Goliath server.

zond commented Aug 27, 2012

@jonhyman, I don't, we only used this to enable EM connections in Savon so as not to block our Goliath server.

@jonhyman

This comment has been minimized.

Show comment
Hide comment
@jonhyman

jonhyman Aug 27, 2012

Thanks. It seems like you can get the error by catching an exception. Here's a sample script I've been playing with in my console that illustrates it. I think it works this way because it's dependent on fibers (e.g., you can't run HTTPI methods inside an EM.run when using :em_http, it has to be EM.synchrony)

# Note the invalid url for techcrunch...
urls = ["http://www.google.com", "http://www.yahoo.com", "http://www.techcrunch.c"]
results = []
concurrency = 3
HTTPI::Adapter.use = :em_http
EM.synchrony do
  EM::Timer.new(2) {EM.stop}
  EM::Synchrony::FiberIterator.new(urls, concurrency).each do |url|
    r = HTTPI::Request.new
    r.url = url
    begin
      resp = HTTPI.get(r)
      results.push(resp)
    rescue Exception => e
      puts("Error for #{url}")
    end
  end
end

Thanks. It seems like you can get the error by catching an exception. Here's a sample script I've been playing with in my console that illustrates it. I think it works this way because it's dependent on fibers (e.g., you can't run HTTPI methods inside an EM.run when using :em_http, it has to be EM.synchrony)

# Note the invalid url for techcrunch...
urls = ["http://www.google.com", "http://www.yahoo.com", "http://www.techcrunch.c"]
results = []
concurrency = 3
HTTPI::Adapter.use = :em_http
EM.synchrony do
  EM::Timer.new(2) {EM.stop}
  EM::Synchrony::FiberIterator.new(urls, concurrency).each do |url|
    r = HTTPI::Request.new
    r.url = url
    begin
      resp = HTTPI.get(r)
      results.push(resp)
    rescue Exception => e
      puts("Error for #{url}")
    end
  end
end
@zond

This comment has been minimized.

Show comment
Hide comment
@zond

zond Aug 27, 2012

@jonhyman, yeah that might be true. We only used it inside EM.synchrony.

Anyway, it's a long time since I touched those things. Good luck :D

zond commented Aug 27, 2012

@jonhyman, yeah that might be true. We only used it inside EM.synchrony.

Anyway, it's a long time since I touched those things. Good luck :D

@turboladen

This comment has been minimized.

Show comment
Hide comment
@turboladen

turboladen Oct 4, 2012

Contributor

@rubiii I finally got back to checking in to those failures and I can't get any unit tests to fail for me. (I also don't seem to find the failures on Travis anymore, due to your org change.) I wonder if those old failures might have something to do with a dependency that's now been updated? In any case, I forked your em_http branch and all unit tests were passing, but integration tests were not.

I have the following rubies installed:

  • jruby-1.6.8 [ x86_64 ]
  • rbx-head [ x86_64 ]
  • ree-1.8.7-2012.02 [ i686 ]
  • ruby-1.8.7-p370 [ i686 ]
  • ruby-1.9.2-p320 [ x86_64 ]
  • ruby-1.9.3-p194 [ x86_64 ]

This resulted in all green before and after my changes:

  • rvm all do rake spec

This resulted in failures before my changes, but now passes:

  • rvm all do rake spec_integration

https://github.com/turboladen/httpi/compare/savonrb:em_http...turboladen:em_http. Can you let me know if you're willing to take a pull request?

Contributor

turboladen commented Oct 4, 2012

@rubiii I finally got back to checking in to those failures and I can't get any unit tests to fail for me. (I also don't seem to find the failures on Travis anymore, due to your org change.) I wonder if those old failures might have something to do with a dependency that's now been updated? In any case, I forked your em_http branch and all unit tests were passing, but integration tests were not.

I have the following rubies installed:

  • jruby-1.6.8 [ x86_64 ]
  • rbx-head [ x86_64 ]
  • ree-1.8.7-2012.02 [ i686 ]
  • ruby-1.8.7-p370 [ i686 ]
  • ruby-1.9.2-p320 [ x86_64 ]
  • ruby-1.9.3-p194 [ x86_64 ]

This resulted in all green before and after my changes:

  • rvm all do rake spec

This resulted in failures before my changes, but now passes:

  • rvm all do rake spec_integration

https://github.com/turboladen/httpi/compare/savonrb:em_http...turboladen:em_http. Can you let me know if you're willing to take a pull request?

@rubiii

This comment has been minimized.

Show comment
Hide comment
@rubiii

rubiii Oct 5, 2012

Contributor

sure.

Contributor

rubiii commented Oct 5, 2012

sure.

@rubiii

This comment has been minimized.

Show comment
Hide comment
@rubiii

rubiii Oct 12, 2012

Contributor

hey. i just merged support for em_http into master. please give it a try before it's going to be released.

Contributor

rubiii commented Oct 12, 2012

hey. i just merged support for em_http into master. please give it a try before it's going to be released.

@turboladen

This comment has been minimized.

Show comment
Hide comment
@turboladen

turboladen Oct 12, 2012

Contributor

I'm not sure how I'd handle this, but perhaps you've got a stance on it already...

When I use it from a project of mine, I get:

WARNING: unable to load thorfile "/Users/Steveloveless/Development/projects/upnp/tasks/control_point.thor": cannot load such file -- em-synchrony
/Users/Steveloveless/Development/projects/httpi_read_only/lib/httpi/adapter.rb:49:in `require'

My gem doesn't use em-synchrony (and imagine that a good portion of EventMachine users aren't), but the adapter does; this is resolved, of course, if I add em-synchrony to my project. Maybe this should be added to the docs somewhere? I can do that if you can tell me where...

Secondly, my gem makes EM::HttpRequest calls (which is used by the adapter) using the standard EventMachine way--these calls are now failing with FiberErrors. This tells me that I need to make changes to my calls to do them em-synchrony style. Again, this still works for me (I think--I've never used em-syncrhony), but seems like it should be documented.

FWIW, I made an attempt to remove the em-synchrony dep from the adapter a couple weeks ago, but couldn't figure out how to make it all work. At that point, I undid my changes, and decided I was happy to make my code em-sychnrony compatible, just so I can have an EventMachine compatible HTTPI.

Contributor

turboladen commented Oct 12, 2012

I'm not sure how I'd handle this, but perhaps you've got a stance on it already...

When I use it from a project of mine, I get:

WARNING: unable to load thorfile "/Users/Steveloveless/Development/projects/upnp/tasks/control_point.thor": cannot load such file -- em-synchrony
/Users/Steveloveless/Development/projects/httpi_read_only/lib/httpi/adapter.rb:49:in `require'

My gem doesn't use em-synchrony (and imagine that a good portion of EventMachine users aren't), but the adapter does; this is resolved, of course, if I add em-synchrony to my project. Maybe this should be added to the docs somewhere? I can do that if you can tell me where...

Secondly, my gem makes EM::HttpRequest calls (which is used by the adapter) using the standard EventMachine way--these calls are now failing with FiberErrors. This tells me that I need to make changes to my calls to do them em-synchrony style. Again, this still works for me (I think--I've never used em-syncrhony), but seems like it should be documented.

FWIW, I made an attempt to remove the em-synchrony dep from the adapter a couple weeks ago, but couldn't figure out how to make it all work. At that point, I undid my changes, and decided I was happy to make my code em-sychnrony compatible, just so I can have an EventMachine compatible HTTPI.

@rubiii

This comment has been minimized.

Show comment
Hide comment
@rubiii

rubiii Oct 13, 2012

Contributor

i'm not sure if we need em-synchrony. it sounds like a good idea to use fibers, but i'm not sure.
i'm currently updating the documentation for the next release and will add some detailed information about this.
for now, i simply added a note to the readme. hope that's ok.

Contributor

rubiii commented Oct 13, 2012

i'm not sure if we need em-synchrony. it sounds like a good idea to use fibers, but i'm not sure.
i'm currently updating the documentation for the next release and will add some detailed information about this.
for now, i simply added a note to the readme. hope that's ok.

@turboladen

This comment has been minimized.

Show comment
Hide comment
@turboladen

turboladen Oct 15, 2012

Contributor

Well, the adapter seems like it was designed with em-synchrony in mind; if it wasn't, I'm pretty sure there'd be a bunch of callbacks/errbacks defined and the flow would probably be a bit different. I've only been working with EM for a few months, so I'm no pro here though.

It'd be great if em-sychrony wasn't a requirement, but maybe that's just something to change for the future? That would also open up compatibility to 1.8.x rubies... In any case, just documenting works great for me for now. Thanks for getting this in.

Contributor

turboladen commented Oct 15, 2012

Well, the adapter seems like it was designed with em-synchrony in mind; if it wasn't, I'm pretty sure there'd be a bunch of callbacks/errbacks defined and the flow would probably be a bit different. I've only been working with EM for a few months, so I'm no pro here though.

It'd be great if em-sychrony wasn't a requirement, but maybe that's just something to change for the future? That would also open up compatibility to 1.8.x rubies... In any case, just documenting works great for me for now. Thanks for getting this in.

@rubiii

This comment has been minimized.

Show comment
Hide comment
@rubiii

rubiii Oct 20, 2012

Contributor

hey guys. i added some integration spec (3904d3) to verify ssl authentication and it seems like the em_http adapter doesn't support this feature at all (see #65). so i'm wondering where the information about configuring ssl for the em_http adapter came from.

it seems more appropriate to remove that method and raise an error when someone tries to use ssl authentication with em_http, because the ssl settings seem to be ignored?!

/cc @RISCfuture

Contributor

rubiii commented Oct 20, 2012

hey guys. i added some integration spec (3904d3) to verify ssl authentication and it seems like the em_http adapter doesn't support this feature at all (see #65). so i'm wondering where the information about configuring ssl for the em_http adapter came from.

it seems more appropriate to remove that method and raise an error when someone tries to use ssl authentication with em_http, because the ssl settings seem to be ignored?!

/cc @RISCfuture

@turboladen

This comment has been minimized.

Show comment
Hide comment
@turboladen

turboladen Nov 1, 2012

Contributor

I've never used it, but search for ssl here and that seems to show that those options are valid.

As far as I'm concerned, I'm fine without SSL support for this; your suggestion for raising if someone tries to use it seems great.

Contributor

turboladen commented Nov 1, 2012

I've never used it, but search for ssl here and that seems to show that those options are valid.

As far as I'm concerned, I'm fine without SSL support for this; your suggestion for raising if someone tries to use it seems great.

@rubiii

This comment has been minimized.

Show comment
Hide comment
@rubiii

rubiii Nov 10, 2012

Contributor

i'm quite busy right now, so sorry for the delay. i just released 2.0.0.rc1 so you can test the new adapter before i'm releasing a stable version. thanks!

Contributor

rubiii commented Nov 10, 2012

i'm quite busy right now, so sorry for the delay. i just released 2.0.0.rc1 so you can test the new adapter before i'm releasing a stable version. thanks!

@rubiii

This comment has been minimized.

Show comment
Hide comment
@rubiii

rubiii Dec 17, 2012

Contributor

released this feature with version 2.0.0. final just now. if you're using httpi with savon,
you can try savon 2.0 which was updated to use the new httpi version.

thanks for your help!

Contributor

rubiii commented Dec 17, 2012

released this feature with version 2.0.0. final just now. if you're using httpi with savon,
you can try savon 2.0 which was updated to use the new httpi version.

thanks for your help!

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