From: Remi Gacogne Date: Wed, 12 Jul 2023 12:41:31 +0000 (+0200) Subject: dnsdist: Simplify I/O handling for incoming H2 w/ nghttp2 X-Git-Tag: rec-5.0.0-alpha1~19^2~19 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=67b9320ece9845521a1e877b96181c8f373eed98;p=thirdparty%2Fpdns.git dnsdist: Simplify I/O handling for incoming H2 w/ nghttp2 --- diff --git a/pdns/dnsdist-doh-common.hh b/pdns/dnsdist-doh-common.hh index f0a1adc767..6e0cc86e03 100644 --- a/pdns/dnsdist-doh-common.hh +++ b/pdns/dnsdist-doh-common.hh @@ -77,7 +77,7 @@ struct DOHFrontend DOHFrontend() { } - DOHFrontend(std::shared_ptr tlsCtx): + DOHFrontend(std::shared_ptr tlsCtx) : d_tlsContext(std::move(tlsCtx)) { } diff --git a/pdns/dnsdistdist/dnsdist-nghttp2-in.cc b/pdns/dnsdistdist/dnsdist-nghttp2-in.cc index b71601a916..7945a2544f 100644 --- a/pdns/dnsdistdist/dnsdist-nghttp2-in.cc +++ b/pdns/dnsdistdist/dnsdist-nghttp2-in.cc @@ -375,11 +375,6 @@ void IncomingHTTP2Connection::handleIO() if (nghttp2_session_want_read(d_session.get()) != 0) { updateIO(IOState::NeedRead, handleReadableIOCallback); } - else { - if (isIdle()) { - watchForRemoteHostClosingConnection(); - } - } } } catch (const std::exception& e) { @@ -400,12 +395,7 @@ void IncomingHTTP2Connection::writeToSocket(bool socketReady) d_out.clear(); d_outPos = 0; if (active() && !d_connectionClosing) { - if (!isIdle()) { - updateIO(IOState::NeedRead, handleReadableIOCallback); - } - else { - watchForRemoteHostClosingConnection(); - } + updateIO(IOState::NeedRead, handleReadableIOCallback); } else { stopIO(); @@ -791,8 +781,7 @@ void IncomingHTTP2Connection::handleIncomingQuery(IncomingHTTP2Connection::Pendi sendResponse(streamID, query, code, d_ci.cs->dohFrontend->d_customResponseHeaders); }; - if (query.d_method == PendingQuery::Method::Unknown || - query.d_method == PendingQuery::Method::Unsupported) { + if (query.d_method == PendingQuery::Method::Unknown || query.d_method == PendingQuery::Method::Unsupported) { handleImmediateResponse(400, "DoH query not allowed because of unsupported HTTP method"); return; } @@ -919,10 +908,6 @@ int IncomingHTTP2Connection::on_frame_recv_callback(nghttp2_session* session, co auto stream = conn->d_currentStreams.find(streamID); if (stream != conn->d_currentStreams.end()) { conn->handleIncomingQuery(std::move(stream->second), streamID); - - if (conn->isIdle()) { - conn->watchForRemoteHostClosingConnection(); - } } else { vinfolog("Stream %d NOT FOUND", streamID); @@ -954,10 +939,6 @@ int IncomingHTTP2Connection::on_stream_close_callback(nghttp2_session* session, auto request = std::move(stream->second); conn->d_currentStreams.erase(stream->first); - if (conn->isIdle()) { - conn->watchForRemoteHostClosingConnection(); - } - return 0; } @@ -1222,20 +1203,6 @@ void IncomingHTTP2Connection::updateIO(IOState newState, const FDMultiplexer::ca } } -void IncomingHTTP2Connection::watchForRemoteHostClosingConnection() -{ - if (d_connectionDied) { - return; - } - - if (hasPendingWrite()) { - updateIO(IOState::NeedWrite, &handleWritableIOCallback); - } - else if (!d_connectionClosing) { - updateIO(IOState::NeedRead, handleReadableIOCallback); - } -} - void IncomingHTTP2Connection::handleIOError() { d_connectionDied = true; diff --git a/pdns/dnsdistdist/dnsdist-nghttp2-in.hh b/pdns/dnsdistdist/dnsdist-nghttp2-in.hh index 3db7473a8e..32af8d3087 100644 --- a/pdns/dnsdistdist/dnsdist-nghttp2-in.hh +++ b/pdns/dnsdistdist/dnsdist-nghttp2-in.hh @@ -89,7 +89,6 @@ private: bool isIdle() const; uint32_t getConcurrentStreamsCount() const; void updateIO(IOState newState, const FDMultiplexer::callbackfunc_t& callback); - void watchForRemoteHostClosingConnection(); void handleIOError(); bool sendResponse(StreamID streamID, PendingQuery& context, uint16_t responseCode, const HeadersMap& customResponseHeaders, const std::string& contentType = "", bool addContentType = true); void handleIncomingQuery(PendingQuery&& query, StreamID streamID); diff --git a/pdns/dnsdistdist/test-dnsdistnghttp2-in_cc.cc b/pdns/dnsdistdist/test-dnsdistnghttp2-in_cc.cc index 0ac62b3c3b..48d2f6eeeb 100644 --- a/pdns/dnsdistdist/test-dnsdistnghttp2-in_cc.cc +++ b/pdns/dnsdistdist/test-dnsdistnghttp2-in_cc.cc @@ -90,7 +90,7 @@ public: { const auto& context = s_connectionContexts.at(connectionID); d_clientOutBuffer.insert(d_clientOutBuffer.begin(), context.d_proxyProtocolPayload.begin(), context.d_proxyProtocolPayload.end()); - + nghttp2_session_callbacks* cbs = nullptr; nghttp2_session_callbacks_new(&cbs); std::unique_ptr callbacks(cbs, nghttp2_session_callbacks_del); @@ -499,19 +499,19 @@ BOOST_FIXTURE_TEST_CASE(test_IncomingConnection_SelfAnswered, TestFixture) s_connectionContexts[counter++] = ExpectedData{{}, {query}, {response}, {403U}}; s_steps = { /* opening */ - { ExpectedStep::ExpectedRequest::handshakeClient, IOState::Done }, + {ExpectedStep::ExpectedRequest::handshakeClient, IOState::Done}, /* settings server -> client */ - { ExpectedStep::ExpectedRequest::writeToClient, IOState::Done, 15 }, + {ExpectedStep::ExpectedRequest::writeToClient, IOState::Done, 15}, /* settings + headers + data client -> server.. */ - { ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 128 }, + {ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 128}, /* .. continued */ - { ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 60 }, + {ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 60}, /* headers + data */ - { ExpectedStep::ExpectedRequest::writeToClient, IOState::Done, std::numeric_limits::max() }, + {ExpectedStep::ExpectedRequest::writeToClient, IOState::Done, std::numeric_limits::max()}, /* wait for next query, but the client closes the connection */ - { ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 0 }, + {ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 0}, /* server close */ - { ExpectedStep::ExpectedRequest::closeClient, IOState::Done }, + {ExpectedStep::ExpectedRequest::closeClient, IOState::Done}, }; auto state = std::make_shared(ConnectionInfo(&localCS, getBackendAddress("84", 4242)), threadData, now); @@ -520,18 +520,18 @@ BOOST_FIXTURE_TEST_CASE(test_IncomingConnection_SelfAnswered, TestFixture) { /* client closes the connection right in the middle of sending the query */ - s_connectionContexts[counter++] = ExpectedData{{}, {query}, {response}, { 403U }}; + s_connectionContexts[counter++] = ExpectedData{{}, {query}, {response}, {403U}}; s_steps = { /* opening */ - { ExpectedStep::ExpectedRequest::handshakeClient, IOState::Done }, + {ExpectedStep::ExpectedRequest::handshakeClient, IOState::Done}, /* settings server -> client */ - { ExpectedStep::ExpectedRequest::writeToClient, IOState::Done, 15 }, + {ExpectedStep::ExpectedRequest::writeToClient, IOState::Done, 15}, /* client sends one byte */ - { ExpectedStep::ExpectedRequest::readFromClient, IOState::NeedRead, 1 }, + {ExpectedStep::ExpectedRequest::readFromClient, IOState::NeedRead, 1}, /* then closes the connection */ - { ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 0 }, + {ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 0}, /* server close */ - { ExpectedStep::ExpectedRequest::closeClient, IOState::Done }, + {ExpectedStep::ExpectedRequest::closeClient, IOState::Done}, }; /* mark the incoming FD as always ready */ @@ -556,19 +556,19 @@ BOOST_FIXTURE_TEST_CASE(test_IncomingConnection_SelfAnswered, TestFixture) s_steps = { /* opening */ - { ExpectedStep::ExpectedRequest::handshakeClient, IOState::Done }, + {ExpectedStep::ExpectedRequest::handshakeClient, IOState::Done}, /* settings server -> client */ - { ExpectedStep::ExpectedRequest::writeToClient, IOState::Done, 15 }, + {ExpectedStep::ExpectedRequest::writeToClient, IOState::Done, 15}, /* settings + headers + data client -> server.. */ - { ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 128 }, + {ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 128}, /* .. continued */ - { ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 60 }, + {ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 60}, /* headers + data */ - { ExpectedStep::ExpectedRequest::writeToClient, IOState::Done, std::numeric_limits::max() }, + {ExpectedStep::ExpectedRequest::writeToClient, IOState::Done, std::numeric_limits::max()}, /* wait for next query, but the client closes the connection */ - { ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 0 }, + {ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 0}, /* server close */ - { ExpectedStep::ExpectedRequest::closeClient, IOState::Done }, + {ExpectedStep::ExpectedRequest::closeClient, IOState::Done}, }; auto state = std::make_shared(ConnectionInfo(&localCS, getBackendAddress("84", 4242)), threadData, now); @@ -587,17 +587,17 @@ BOOST_FIXTURE_TEST_CASE(test_IncomingConnection_SelfAnswered, TestFixture) s_steps = { /* opening */ - { ExpectedStep::ExpectedRequest::handshakeClient, IOState::Done }, + {ExpectedStep::ExpectedRequest::handshakeClient, IOState::Done}, /* settings server -> client */ - { ExpectedStep::ExpectedRequest::writeToClient, IOState::Done, 15 }, + {ExpectedStep::ExpectedRequest::writeToClient, IOState::Done, 15}, /* settings + headers + data client -> server.. */ - { ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 128 }, + {ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 128}, /* .. continued */ - { ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 60 }, + {ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 60}, /* we want to send the response but the client closes the connection */ - { ExpectedStep::ExpectedRequest::writeToClient, IOState::Done, 0 }, + {ExpectedStep::ExpectedRequest::writeToClient, IOState::Done, 0}, /* server close */ - { ExpectedStep::ExpectedRequest::closeClient, IOState::Done }, + {ExpectedStep::ExpectedRequest::closeClient, IOState::Done}, }; /* mark the incoming FD as always ready */ @@ -622,21 +622,21 @@ BOOST_FIXTURE_TEST_CASE(test_IncomingConnection_SelfAnswered, TestFixture) s_steps = { /* opening */ - { ExpectedStep::ExpectedRequest::handshakeClient, IOState::Done }, + {ExpectedStep::ExpectedRequest::handshakeClient, IOState::Done}, /* settings server -> client */ - { ExpectedStep::ExpectedRequest::writeToClient, IOState::Done, 15 }, + {ExpectedStep::ExpectedRequest::writeToClient, IOState::Done, 15}, /* settings + headers + data client -> server.. */ - { ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 128 }, + {ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 128}, /* .. continued */ - { ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 60 }, + {ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 60}, /* headers + data (partial write) */ - { ExpectedStep::ExpectedRequest::writeToClient, IOState::NeedWrite, 1 }, + {ExpectedStep::ExpectedRequest::writeToClient, IOState::NeedWrite, 1}, /* nothing to read after that */ - { ExpectedStep::ExpectedRequest::readFromClient, IOState::NeedRead, 0 }, + {ExpectedStep::ExpectedRequest::readFromClient, IOState::NeedRead, 0}, /* then the client closes the connection before we are done */ - { ExpectedStep::ExpectedRequest::writeToClient, IOState::Done, 0 }, + {ExpectedStep::ExpectedRequest::writeToClient, IOState::Done, 0}, /* server close */ - { ExpectedStep::ExpectedRequest::closeClient, IOState::Done }, + {ExpectedStep::ExpectedRequest::closeClient, IOState::Done}, }; /* mark the incoming FD as always ready */ @@ -691,25 +691,24 @@ BOOST_FIXTURE_TEST_CASE(test_IncomingConnection_BackendTimeout, TestFixture) s_connectionContexts[counter++] = ExpectedData{{}, {query}, {response}, {502U}}; s_steps = { /* opening */ - { ExpectedStep::ExpectedRequest::handshakeClient, IOState::Done }, + {ExpectedStep::ExpectedRequest::handshakeClient, IOState::Done}, /* settings server -> client */ - { ExpectedStep::ExpectedRequest::writeToClient, IOState::Done, 15 }, + {ExpectedStep::ExpectedRequest::writeToClient, IOState::Done, 15}, /* settings + headers + data client -> server.. */ - { ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 128 }, + {ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 128}, /* .. continued */ - { ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 60 }, - /* trying to read a new request while processing the first one */ - { ExpectedStep::ExpectedRequest::readFromClient, IOState::NeedRead }, + {ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 60}, + /* trying to read a new request while processing the first one */ + {ExpectedStep::ExpectedRequest::readFromClient, IOState::NeedRead}, /* headers + data */ - { ExpectedStep::ExpectedRequest::writeToClient, IOState::Done, std::numeric_limits::max(), [&threadData](int desc) { - /* set the incoming descriptor as ready */ - dynamic_cast(threadData.mplexer.get())->setReady(desc); - } - }, + {ExpectedStep::ExpectedRequest::writeToClient, IOState::Done, std::numeric_limits::max(), [&threadData](int desc) { + /* set the incoming descriptor as ready */ + dynamic_cast(threadData.mplexer.get())->setReady(desc); + }}, /* wait for next query, but the client closes the connection */ - { ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 0 }, + {ExpectedStep::ExpectedRequest::readFromClient, IOState::Done, 0}, /* server close */ - { ExpectedStep::ExpectedRequest::closeClient, IOState::Done }, + {ExpectedStep::ExpectedRequest::closeClient, IOState::Done}, }; auto state = std::make_shared(ConnectionInfo(&localCS, getBackendAddress("84", 4242)), threadData, now);