Ensure graceful closure of tracer resources #1334
Conversation
spec/ddtrace_integration_spec.rb
Outdated
| end | ||
|
|
||
| it 'closes tracer file descriptors' do | ||
| try_wait_until { thread_count > original_thread_count } |
There was a problem hiding this comment.
hmm... interesting...
I think I get why you didn't add this in originally
|
CI is hard, I'm still working on getting it green. |
Codecov Report
@@ Coverage Diff @@
## master #1334 +/- ##
=======================================
Coverage 98.16% 98.16%
=======================================
Files 768 769 +1
Lines 36667 36698 +31
=======================================
+ Hits 35993 36024 +31
Misses 674 674
Continue to review full report at Codecov.
|
spec/ddtrace_integration_spec.rb
Outdated
| before do | ||
| original_thread_count | ||
| end | ||
|
|
||
| let(:original_thread_count) { thread_count } |
There was a problem hiding this comment.
Minor: RSpec provides a let! for exactly this behavior -- a let that always runs before the testcase :) (Docs link: https://relishapp.com/rspec/rspec-core/v/3-10/docs/helper-methods/let-and-let)
There was a problem hiding this comment.
I wanted to make sure that it was called before start_tracer, and it looked like this before my latest commit:
before do
start_tracer
original_thread_count
end
I think I refactored it in the last one and didn't see the lonely original_thread_count there.
spec/ddtrace_integration_spec.rb
Outdated
| start_tracer | ||
| end | ||
|
|
||
| let(:original_fd_count) { fd_count } | ||
|
|
||
| def fd_count | ||
| Dir['/dev/fd/*'].size | ||
| end | ||
|
|
||
| it 'closes tracer file descriptors' do | ||
| start_tracer |
There was a problem hiding this comment.
Minor: start_tracer is getting called twice (this seems unintended...?)
There was a problem hiding this comment.
Unlikely to be intentional 😅
spec/ddtrace_integration_spec.rb
Outdated
| def fd_count | ||
| Dir['/dev/fd/*'].size | ||
| end |
There was a problem hiding this comment.
Since this is a bit of unix black magic, I suggest leaving here a comment explaining a bit what this is and how it works :)
|
|
spec/ddtrace_integration_spec.rb
Outdated
| before do | ||
| # Ensure tracer is not running before test starts. | ||
| # This shouldn't be necessary if all other tests | ||
| # always terminate the tracer. | ||
| Datadog.shutdown! | ||
| end |
There was a problem hiding this comment.
I don't quite like making the tests being defensive in this way, e.g. if the tracer shouldn't stay around between tests, I'd favor something like the thread leak detector where we blame the test that forgot to clean up, rather than make other tests need to clean up after it :)
There was a problem hiding this comment.
We have a Datadog.shutdown! helper that runs after every test: https://github.com/DataDog/dd-trace-rb/blob/master/spec/spec_helper.rb#L157-L169
This before block introduced here can only catch issues that happen if the tracer was initialized outside of the scope of an RSpec test run, which seems to be happening.
I added it because I noticed that one of the tests this PR was flaky without it, and I leaned towards non-flakiness at the expense of masking the root cause. I'll revert this change then, and see if we can spot the root cause at a later time.
There was a problem hiding this comment.
The missing shutdown was in the same file: 50c9a0b (#1334). Fixed it.
This PR adds integration tests to assert that no resources are being leaked after the tracer shuts down.
As we register the tracer to be gracefully shut down at process exit, we should ensure that all resources are correctly being handled and closed, to avoid any possible data loss.