-
Notifications
You must be signed in to change notification settings - Fork 52
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
Comments
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:
What do you think? |
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 |
Okay, I guess that makes sense. So were are you proposing Stamps::API.initialize should go? |
it already exists :) https://github.com/mattsears/stamps/blob/master/lib/stamps/api.rb#L12 |
Right, I mean what changes are you proposing? :-) |
oh, just basically declaring a 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. |
Gotcha. Enjoy the rest of your vacation! |
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?
The text was updated successfully, but these errors were encountered: