mattsears / stamps

Print stamps with Ruby!
https://github.com/mattsears/stamps
MIT License
47 stars 52 forks source link

Savon client reuse? #9

Open bcm opened 12 years ago

bcm commented 12 years ago

According to my reading of Stamps::Request, you're creating an instance of Savon::Client for each request. Is there any particular reason for that?

Savon uses HTTPI ,which in turn prefers HTTPClient, which is documented as having an expensive initialization procedure. The recommended method of using HTTPClient is to set up a single client instance and then reuse it across requests (and threads, as it's threadsafe).

So, it'd be preferable for the Stamps client, during initialization, to initialize a Savon client (and potentially the underlying HTTPI adapter, since Savon doesn't appear to do that itself). This would keep us from incurring the setup cost on every request.

Thoughts?

mattsears commented 12 years ago

Ah, I see. Yeah, I guess it would make sense to initialize Savon::Client just once. We could probably set up the client as an instance variable and use memoization. Something like this perhaps:

def savon
  @savon ||= Savon::Client.new do |wsdl, http|
        wsdl.endpoint = self.endpoint
        wsdl.namespace = self.namespace
   end
end

What do you think?

bcm commented 12 years ago

I'd want to initialize the client before the component using Stamps is ready for service in order to avoid race conditions. so for example, at component start up time, it would call Stamps.client, and then in turn perhaps Stamps::API.initialize would instantiate the Savon::Client. From then on, access by multiple threads would be totally safe.

mattsears commented 12 years ago

Okay, I guess that makes sense. So were are you proposing Stamps::API.initialize should go?

bcm commented 12 years ago

it already exists :) https://github.com/mattsears/stamps/blob/master/lib/stamps/api.rb#L12

mattsears commented 12 years ago

Right, I mean what changes are you proposing? :-)

bcm commented 12 years ago

oh, just basically declaring a savon attribute in Stamps::API and moving that block you indicated earlier in the discussion into Stamps::API.initialize.

I don't have time to work up a patch at the moment. when I get back from vacation next weekend I'll try to do that, as well as to wrap up the various other patches I've made to my fork and submit them upstream.

mattsears commented 12 years ago

Gotcha. Enjoy the rest of your vacation!