Savon::Client and threadsafety

[iain]

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? 🤘

[rubiii]

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.

[kalarani]

@rubiii how does this work with Savon::Model. We don't have control over Savon::Client creation. It is created and memoized, hence it'd be the same for all the threads. Checkout my gist and call the thread_safe? method to simulate the issue.

[timabdulla]

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.

[rubiii]

can you specify the state that is causing problems and help to fix this?

[timabdulla]

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.)

[rubiii]

thanks for explaining. your approach sounds like a good solution and i will definitely accept your pull request.
thanks in advance!

[timabdulla]

great! should be done either today or tomorrow...

[timabdulla]

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.

[rubiii]

released with v1.2.0.