Merge pull request #2798 from newrelic/jruby_fix

JRuby fix for concurrency issue with manager cache

CHANGELOG.md

@@ -2,7 +2,7 @@
 
 ## dev
 
-Version <dev> updates framework detection and fixes Falcon dispatcher detection.
+Version <dev> updates framework detection, fixes Falcon dispatcher detection, and addresses a JRuby specific concurrency issue.
 
 - **Feature: Improve framework detection accuracy for Grape and Padrino**
 
@@ -12,6 +12,10 @@ Version <dev> updates framework detection and fixes Falcon dispatcher detection.
 
   Previously, we tried to use the object space to determine whether the [Falcon web server](https://github.com/socketry/falcon) was in use. However, Falcon is not added to the object space until after the environment report is generated, resulting in a `nil` dispatcher. Now, we revert to an earlier strategy that discovered the dispatcher using `File.basename`. Thank you, [@prateeksen](https://github.com/prateeksen) for reporting this issue and researching the problem. [Issue#2778](https://github.com/newrelic/newrelic-ruby-agent/issues/2778) [PR#2795](https://github.com/newrelic/newrelic-ruby-agent/pull/2795)
 
+- **Bugfix: Address JRuby concurrency issue with config hash accessing**
+
+  The agent's internal configuration class maintains a hash that occassionally gets rebuilt. During the rebuild, certain previously dynamically determined instrumentation values are preserved for the benefit of the [New Relic Ruby security agent](https://github.com/newrelic/csec-ruby-agent). After reports from JRuby customers regarding concurrency issues related to the hash being accessed while being modified, two separate fixes went into the hash rebuild logic previously: a `Hash#dup` operation and a `synchronize do` block. But errors were still reported. We ourselves remain unable to reproduce these concurrency errors despite using the same exact versions of JRuby and all reported software. After confirming that the hash access code in question is only needed for the Ruby security agent (which operates only in non-production dedicated security testing environments), we have introduced a new fix for JRuby customers that will simply skip over the troublesome code when JRuby is in play but the security agent is not. [PR#2798](https://github.com/newrelic/newrelic-ruby-agent/pull/2798)
+
 ## v9.12.0
 
 Version 9.12.0 adds support for the `newrelic_security` agent, introduces instrumentation for the LogStasher gem, improves instrumentation for the `redis-clustering` gem, and updates the Elasticsearch instrumentation to only attempt to get the cluster name once per client, even if it fails.

lib/new_relic/agent/configuration/manager.rb

@@ -382,6 +382,14 @@ def reset_to_defaults
         def reset_cache
           return new_cache unless defined?(@cache) && @cache
 
+          # Modifying the @cache hash under JRuby - even with a `synchronize do`
+          # block and a `Hash#dup` operation - has been known to cause issues
+          # with JRuby for concurrent access of the hash while it is being
+          # modified. The hash really only needs to be modified for the benefit
+          # of the security agent, so if JRuby is in play and the security agent
+          # is not, don't attempt to modify the hash at all and return early.
+          return @cache if NewRelic::LanguageSupport.jruby? && !Agent.config[:'security.agent.enabled']
+
           @lock.synchronize do
             preserved = @cache.dup.select { |_k, v| DEPENDENCY_DETECTION_VALUES.include?(v) }
             new_cache

test/new_relic/agent/configuration/manager_test.rb

@@ -537,6 +537,19 @@ def test_logger_does_not_receive_excluded_settings
       refute_includes(log, ':license_key')
     end
 
+    def test_reset_cache_return_early_for_jruby
+      phony_cache = {dup_called: false}
+      def phony_cache.dup; self[:dup_called] = true; self; end
+      @manager.instance_variable_set(:@cache, phony_cache)
+      NewRelic::LanguageSupport.stub :jruby?, true do
+        @manager.reset_cache
+      end
+
+      refute phony_cache[:dup_called], 'Expected the use of JRuby to prevent the Hash#dup call!'
+    ensure
+      @manager.new_cache
+    end
+
     private
 
     def assert_parsed_labels(expected)