Fixed a threading related issue that occurs because $1 is not thread-safe when used from lambdas

[perlun]

See #29. I must admit that I cannot promise 100% equivalence between my approach and the original one. But all specs pass with these changes also, so I guess they cannot be that bad... 😃

[tjarratt]

Like the old adage says, if the specs pass, you must ship it!

Thanks @perlun for the pull request. This change looks sane, simple and (most of all) thread safe.

[kafkasbug]

Just playing around with the newest version:
This function call: Gyoku.xml_tag "hello_worldBob/how_are_you|there:foo^bar".to_sym

Previous version returned: helloWorldBob::HowAreYou|there:foo^bar

Latest version returns: helloWorldBob/howAreYouThere:foo^bar

I'm not certain if this will break things for someone down the road or not, particularly the '/' not getting converted to '::' in the latest version.

Co-worker of mine has put together this hack that we're using:

CAMELCASE = lambda do |key|
key.split('/').map do |e|
e.split('_').map{|e| e[0].chr.upcase + e[1..-1]}.join
end.join("::")

[tjarratt]

Thanks for the heads up @kafkasbug -- it looked sane to me and passed unit tests. Will either pull this version tonight or try some more testing.

Sigh. Removing global state is hard!

[tjarratt]

I was terribly surprised to discover that $1 is not exactly equal to the block variable that gsub provides here. That's ... very disappointing of ruby.

[tjarratt]

Thanks @kafkasbug for the sanity check. I've pulled the version from rubygems and updated it accordingly.

[perlun]

@kafkasbug - good that you noticed. As stated, I did not promise 1-to-1 equivalence between the old and new version. But it did pass the tests... 😄

Would you mind submitting a PR that adds the missing spec(s)? And also, perhaps include your co-worker's hack in there also. For me, it would be really critical to get this bug fixed since it occasionally hits one of our customers in production.

[tjarratt]

Oops, I think my commit message might have been a little misleading. Instead of reverting your changes, @perlun, I actually preserved the original behavior (or, at least, as much as I could from the discussion here).

The current implementation in v1.2.2 looks like
CAMELCASE = lambda { |key| key.gsub(/\/(.?)/) { |m| "::#{m.split('').last.upcase}" }.gsub(/(?:^|_)(.)/) { |m| m.split('').last.upcase } }

So this should be both thread safe and also keep the original behavior. It's a little annoying how we need to pull off the last character each time, but that's how #gsub works given a block, it completely ignores your capture groups.

[perlun]

Ah, excellent! 😄 That's really cool - looking forward to upgrade to 1.2.2 as soon as I find the time for it. And you're right; the fact that $1 and the gsub block parameter isn't equal is extremely annoying (I concluded that also when I did my attempt at fixing it).

"Someone" should also add a spec for this, so we don't break it in the future.

[madkingCoder]

I'm not sure how much performance matters, but I am seeing a 20% speed difference for simple symbols (:a) and more than a 50% difference for symbols with slashes (:"a_b\c_d") when comparing the two solutions on this thread.

CAMELCASE = lambda { |key| key.gsub(/\/(.?)/) { |m| "::#{m.split('').last.upcase}" }.gsub(/(?:^|_)(.)/) { |m| m.split('').last.upcase } }


CAMELCASE = lambda do |key|
    key.split('/').map do |e| 
        e.split('_').map{|e| e[0].chr.upcase + e[1..-1]}.join
    end.join("::")
end

The second one (with no gsub calls) is faster on all my tests. (JRuby 1.7.12)

[tjarratt]

@madkingCoder it's definitely slower on MRI by a factor of 2, but the existing implementation is also simpler to read and doesn't have any (obvious) bugs. The faster code discussed above will crash on empty strings and also double underscores. Those are both easy to solve, but I only found those after staring at the code for 10 minutes. Perhaps there are more edge cases? Hard to say without more investigation.

There are tons and tons of potential optimizations that could be made across Savon, but each one comes with the cost of potentially breaking existing users and making the project harder to maintain. In the absence of any pressing need, I would feel it's fine that camelcasing a single symbol takes twice as long as necessary here.

That said, if someone truly had a pressing need to make Savon/Gyoku more performant, that would be interesting (and I would certainly try to have empathy for their problem), but it would be a massive undertaking. Just making it thread safe for now is a bigger issue, IMO, since we know there are users that run savon under jruby with multiple threads.