Fix memory leak in the curb instrumentation

[charkost]

On long-running connection Curb use-cases like Elasticsearch clients with Curb transport the request variable points to the same Curb object not just for one request but for all.

Therefore, for each succesful request following the first one, original_callback points to the previous on_failure callback set by newrelic. So, an infinite chain of callbacks is created since newrelic's on_failure callback maintains a reference to original_callback.

The changes force original_callback to point to the actual original callback on subsequent requests instead of pointing to the newrelic's callback. That way no reference is maintained for newrelic's callback of the previous request & the memory leak is prevented by the GC.

Before contributing, please read our contributing guidelines and code of conduct.

Overview

Describe the changes present in the pull request

Submitter Checklist:

• Include a link to the related GitHub issue, if applicable
• Include a security review link, if applicable

Testing

The agent includes a suite of unit and functional tests which should be used to
verify your changes don't break existing functionality. These tests will run with
Github Actions when a pull request is made. More details on running the tests locally can be found
here for our unit tests,
and here for our functional tests.
For most contributions it is strongly recommended to add additional tests which
exercise your changes.

Reviewer Checklist

• Perform code review
• Add performance label
• Perform appropriate level of performance testing
• Confirm all checks passed
• Add version label prior to acceptance

[CLAassistant]

All committers have signed the CLA.

[fallwith]

Hi @charkost,

Thank you very much for bringing this issue to our attention and for supplying a fix!

I have one small request for a change to be made in your current implementation.

For all v8.x.x versions of the New Relic Ruby agent, we are still supporting Ruby 2.2 and the safe navigation operator (&.) you used requires Ruby 2.3+. Would you please remove the safe navigation operator and check for nils instead?

Also, I am curious about this part of your PR description:

the request variable points to the same Curb object not just for one request but for all

The original version of the method sets @_nr_failure_instrumented to true on the request object. The next time the same request object is passed to the method, the method should return early when it sees the instance variable set to true. With your fix, you are now adding a new instance variable to the underlying callback object itself. Do you suspect that anything is broken with the original @_nr_failure_instrumented setting/checking behavior? Do we still need that variable on the request object now that your new variable is on the callback object?

[charkost]

Hi @fallwith,

The original version of the method sets @_nr_failure_instrumented to true inside the on_failure callback which means
that it is called only on failed requests. So, if we have a series of only successful requests it will never become true and the method will never return early resulting to the infinite chain of callbacks.

If am not missing something, i think that @_nr_failure_instrumented is completely redundant now.
I could remove it in a new commit if you agree.

[fallwith]

Thank you @charkost, that makes sense.

If am not missing something, i think that @_nr_failure_instrumented is completely redundant now.
I could remove it in a new commit if you agree.

Yes, please. Please remove the redundant check in favor of your new one, and please replace the safe navigation operator (&.) with something Ruby 2.2 compatible, and we'll get this fix merged.

[charkost]

Forced-pushed the following changes:

• Removed &. - instance_variable_get surprisingly works on nils too!
• Replaced Proc.new with proc as suggested by rubocop.
• Removed _nr_failure_instrumented.

[fallwith]

Thank you very much for these updates, @charkost! These changes will be available in our dev branch now and will be distributed with our next agent release.

[fallwith]

Hi @charkost. v8.11.0 of the New Relic Ruby agent has been published to RubyGems and your fix is now live!

[charkost]

Hi @fallwith, thanks for letting me know!