Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Savon client reuse? #9

Open
bcm opened this issue Jul 17, 2012 · 7 comments
Open

Savon client reuse? #9

bcm opened this issue Jul 17, 2012 · 7 comments

Comments

@bcm
Copy link

bcm commented Jul 17, 2012

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
Copy link
Owner

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
Copy link
Author

bcm commented Jul 18, 2012

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
Copy link
Owner

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

@bcm
Copy link
Author

bcm commented Jul 23, 2012

@mattsears
Copy link
Owner

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

@bcm
Copy link
Author

bcm commented Jul 23, 2012

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
Copy link
Owner

Gotcha. Enjoy the rest of your vacation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants