Savon::Client and threadsafety
iain opened this issue · 9 comments
When I have multiple concurrent requests in threads and I use the same instance of Savon::Client, the request body doesn't seem to change between requests.
I have made a small fake SOAP server as you can see here: https://gist.github.com/1725656
And I have code like this:
client = Savon::Client.new do
wsdl.document = "some.wsdl"
wsdl.endpoint = "http://localhost:13010" # my test server
end
threads = some_array.map do |parameters|
Thread.new do
response = client.request(:action_name) { soap.body = parameters }
Thread.current[:value] = response.to_xml
end
end
sleep 3 # some arbitrary timeout, giving the threads time to run
threads.each &:kill
values = threads.map { |thr| thr[:value] }.compact
When I do a bunch of requests simultaneously, the endpoint reports all the same values.
When I put the creation of the client inside the thread, everything works like it should.
I am using Savon 0.9.7 and Ruby 1.9.3.
Also, I explicitly set Nori's parser to Nokogiri, but that didn't help.
Am I doing something wrong? Can you confirm this issue or is it just me? 🤘
i'm pretty much getting the same result for:
client = Savon::Client.new do
wsdl.document = "http://www.thomas-bayer.com/axis2/services/BLZService?wsdl"
end
threads = [70070010, 24050110, 20050550].map do |blz|
Thread.new do
response = client.request :blz, :get_bank, :body => { :blz => blz }
Thread.current[:value] = response.to_hash[:get_bank_response][:details]
end
end
sleep 10
threads.each &:kill
threads.map { |thr| thr[:value] }.compact
the client creates a single request element for every request.
i guess for now you need to create one client per thread.
sorry to reopen an old issue, but the root cause of this problem also manifests itself in some interesting difficulties in single-threaded environments. imo, it is not good for the client to hold state about a request in progress. the client should pass on the actual responsibility of forming and making requests to another class.
can you specify the state that is causing problems and help to fix this?
of course! just had to take some time to ensure that my hypothesis was correct. broadly, the issue is that the Client object maintains state about the request in progress, namely in the @soap
and @http
instance variables.
this necessarily leads to a race condition. let's consider the case of two concurrently executing threads A and B accessing the same Savon client named 'client'.
thread A calls client#request at t1. @soap
is initialized to a new SOAP::XML
object by #request. at t2, thread A is preempted by thread B, which calls client#request. @soap
is once again initialized to a new SOAP::XML
object. let's say that at t3, thread A preempts thread B. its original call to #request has not returned, so it continues to execute. thread A's request is now modifying the state of the SOAP::XML
object that was initialized by thread B. this means that the requests of thread A and thread B are sharing the same SOAP::XML
object stored in @soap
. thusly, we have a race condition, whereby we are unable to determine the final state of @soap
. despite the final state not being deterministic, we do know that thread A and thread B will share the same @soap
. this means that ultimately the requests will be made using the same @soap
.
there is a possibility of thread A or B not being preempted after t2 until they are done making the request, which would allow some level of isolation between requests, but this is exceedingly unlikely.
the second issue is that the @http
variable is per-client, not per-request, which implicitly creates shared state between requests.
what to do? in my opinion, the best and cleanest way to solve this is to create a separate RequestBuilder
class that will handle the creation of the SOAP::Request
object. each call to client#request will result in a separate RequestBuilder
being created. the builder will be stored in a local variable in the scope of the request method. we can isolate our HTTPI::Request
and our SOAP::XML
objects in our RequestBuilder
, as we can move all request building logic out of the client into this separate class. this creates isolation between requests. it's also just a cleaner design. i'm working on doing this refactoring right now. please let me know if this will be ok for a pull request once i'm done. (i'm going to do this anyway because i need these capabilities for a project i'm working on.)
thanks for explaining. your approach sounds like a good solution and i will definitely accept your pull request.
thanks in advance!
great! should be done either today or tomorrow...
quick update on this: i have a build working that is thread safe. i got a bit lazy over the last weekend and it was a holiday here on monday, which meant even more laziness. :-) currently working on updating the tests, then we should be good to go.
released with v1.2.0.