Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rec: drop ref in mtasker when it is no longer needed #14807

Merged
merged 8 commits into from
Nov 5, 2024

Conversation

omoerbeek
Copy link
Member

@omoerbeek omoerbeek commented Oct 28, 2024

Short description

If we do not do that, only the next call of sendEvent will release
the shared pointer to the TCPConnectionHandler in PacketID as it overwrites d_eventkey. Same for d_waitval. This
might delay cleaning TCP objects up on not-so-busy threads.

Fixes #13422, thought it can still take a while on idle recursors,
as the housekeeping function is not called often in those cases.

Added a simple mechanism to make sure d_eventkey is not used when it is not set yet or after being moved.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@omoerbeek omoerbeek changed the title rec: dop packet ref in mtaskler when it is no longer needed rec: drop packet ref in mtasker when it is no longer needed Oct 28, 2024
If we do not do that, only the next call of sendEvent will release
the shared pointer to the TCPConnectionHanlder in PacketID. This
might delay cleaning TCP objects up on not-so-busy threads.

Fixes PowerDNS#13422, thought it can still take a while on idle recursors,
as the housekeeping function is not called often in those cases.
Here no refs being lowered (for the rec case) but it might save a few cycles
@coveralls
Copy link

coveralls commented Oct 28, 2024

Pull Request Test Coverage Report for Build 11665554436

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 62 of 67 (92.54%) changed or added relevant lines in 2 files are covered.
  • 35 unchanged lines in 11 files lost coverage.
  • Overall coverage increased (+0.04%) to 64.661%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pdns/recursordist/mtasker.hh 9 10 90.0%
pdns/recursordist/test-mtasker.cc 53 57 92.98%
Files with Coverage Reduction New Missed Lines %
pdns/recursordist/mtasker.hh 1 88.19%
pdns/tcpiohandler.cc 2 68.2%
pdns/rcpgenerator.cc 2 89.68%
pdns/dnsdistdist/dnsdist-async.cc 2 82.11%
pdns/recursordist/test-syncres_cc2.cc 3 88.85%
pdns/communicator.cc 3 71.85%
pdns/recursordist/syncres.cc 3 79.43%
pdns/communicator.hh 3 77.78%
pdns/opensslsigners.cc 3 61.41%
pdns/dnsdistdist/dnsdist-carbon.cc 6 62.16%
Totals Coverage Status
Change from base Build 11665506881: 0.04%
Covered Lines: 124881
Relevant Lines: 162317

💛 - Coveralls

@omoerbeek omoerbeek changed the title rec: drop packet ref in mtasker when it is no longer needed rec: drop ref in mtasker when it is no longer needed Oct 28, 2024
pdns/recursordist/mtasker.hh Dismissed Show dismissed Hide dismissed
@omoerbeek omoerbeek marked this pull request as ready for review November 1, 2024 10:35
pdns/recursordist/mtasker.hh Show resolved Hide resolved
pdns/recursordist/mtasker.hh Show resolved Hide resolved
Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!
If it's not a lot of work (I have no idea how hard it would be), adding a unit test to ensure that we do not re-introduce the same issue in the future might be a good idea.

@omoerbeek omoerbeek merged commit 19654c1 into PowerDNS:master Nov 5, 2024
78 checks passed
@omoerbeek omoerbeek deleted the rec-packetid-ref branch November 5, 2024 06:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When the recursor is configured to use the TCP protocol for outbound query, TCP fd leakage problem will occur
3 participants