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

dnsdist: Keep watching idle DoH backend connections #10845

Merged

Conversation

rgacogne
Copy link
Member

Short description

So we can quickly detect a connection closed by the remote host. The isTCPSocketUsable() function is unfortunately not enough if the remote end properly sent a GO AWAY frame before closing the connection, as the socket then becomes readable and usable, instead of closed. We need to actually read that message to know that the remote end closed
the connection.

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)

So we can quickly detect a connection closed by the remote host. The
`isTCPSocketUsable()` function is unfortunately not enough if the
remote end properly sent a GO AWAY frame before closing the connection,
as the socket then becomes readable and usable, instead of closed. We
need to actually read that message to know that the remote end closed
the connection.
@rgacogne
Copy link
Member Author

The failure of the recursor regression tests seems unrelated but is quite weird nonetheless, the connections are refused which could indicate a crash of the recursor, but the logs (printed several times apparently?) show no error.

@omoerbeek
Copy link
Member

Weird indeed...

Copy link
Member

@omoerbeek omoerbeek left a comment

Choose a reason for hiding this comment

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

This all reads good, although I do not grasp all the details of the nghttp2 API. I have two nits.

pdns/dnsdistdist/dnsdist-nghttp2.cc Outdated Show resolved Hide resolved
@@ -728,9 +736,16 @@ BOOST_FIXTURE_TEST_CASE(test_SingleQuery, TestFixture)
dynamic_cast<MockupFDMultiplexer*>(s_mplexer.get())->setReady(desc);
}},
/* read settings, headers and response from the server */
{ExpectedStep::ExpectedRequest::readFromBackend, IOState::Done, std::numeric_limits<size_t>::max()},
{ExpectedStep::ExpectedRequest::readFromBackend, IOState::Done, std::numeric_limits<size_t>::max(), [](int desc, const ExpectedStep& step) {
Copy link
Member

Choose a reason for hiding this comment

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

I was looking at the const ExpectedStep& step argument but I could not find any usage of it. I looks redundant to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, it looks like we don't use it in test-dnsdisttcp_cc.cc either, which uses pretty much the same construct. I don't want to remove it in this PR, but I'll do that in a separate one. Thanks!

Co-authored-by: Otto Moerbeek <otto.moerbeek@open-xchange.com>
@rgacogne rgacogne merged commit 9937a1c into PowerDNS:master Oct 25, 2021
@rgacogne rgacogne deleted the ddist-doh-backend-rebased-remote-close branch October 25, 2021 11:42
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.

None yet

2 participants