RequestServer+LibDNS: Don't .await() the DNS lookup promise

...and make sure it will eventually complete (or fail) by adding a
timeout retry sequence.

Fixes an issue where RequestServer would stick around after exit,
waiting for piled up DNS requests for a long time.

Libraries/LibDNS/Resolver.h

@@ -16,6 +16,7 @@
 #include <LibCore/DateTime.h>
 #include <LibCore/Promise.h>
 #include <LibCore/SocketAddress.h>
+#include <LibCore/Timer.h>
 #include <LibDNS/Message.h>
 #include <LibThreading/MutexProtected.h>
 #include <LibThreading/RWLockProtected.h>
@@ -112,6 +113,15 @@ private:
 };
 
 class Resolver {
+    struct PendingLookup {
+        u16 id { 0 };
+        ByteString name;
+        WeakPtr<LookupResult> result;
+        NonnullRefPtr<Core::Promise<NonnullRefPtr<LookupResult const>>> promise;
+        NonnullRefPtr<Core::Timer> repeat_timer;
+        size_t times_repeated { 0 };
+    };
+
 public:
     enum class ConnectionMode {
         TCP,
@@ -203,11 +213,18 @@ public:
         return lookup(move(name), class_, Array { Messages::ResourceType::A, Messages::ResourceType::AAAA });
     }
 
-    NonnullRefPtr<Core::Promise<NonnullRefPtr<LookupResult const>>> lookup(ByteString name, Messages::Class class_, Span<Messages::ResourceType const> desired_types)
+    NonnullRefPtr<Core::Promise<NonnullRefPtr<LookupResult const>>> lookup(ByteString name, Messages::Class class_, Span<Messages::ResourceType const> desired_types, PendingLookup* repeating_lookup = nullptr)
     {
         flush_cache();
 
-        auto promise = Core::Promise<NonnullRefPtr<LookupResult const>>::construct();
+        if (repeating_lookup && repeating_lookup->times_repeated >= 5) {
+            auto promise = repeating_lookup->promise;
+            promise->reject(Error::from_string_literal("DNS lookup timed out"));
+            m_pending_lookups.with_write_locked([&](auto& lookups) { lookups->remove(repeating_lookup->id); });
+            return promise;
+        }
+
+        auto promise = repeating_lookup ? repeating_lookup->promise : Core::Promise<NonnullRefPtr<LookupResult const>>::construct();
 
         if (auto maybe_ipv4 = IPv4Address::from_string(name); maybe_ipv4.has_value()) {
             if (desired_types.contains_slow(Messages::ResourceType::A)) {
@@ -302,11 +319,16 @@ public:
         }
 
         Messages::Message query;
-        m_pending_lookups.with_read_locked([&](auto& lookups) {
-            do
-                fill_with_random({ &query.header.id, sizeof(query.header.id) });
-            while (lookups->find(query.header.id) != nullptr);
-        });
+        if (repeating_lookup) {
+            query.header.id = repeating_lookup->id;
+            repeating_lookup->times_repeated++;
+        } else {
+            m_pending_lookups.with_read_locked([&](auto& lookups) {
+                do
+                    fill_with_random({ &query.header.id, sizeof(query.header.id) });
+                while (lookups->find(query.header.id) != nullptr);
+            });
+        }
         query.header.question_count = max(1u, desired_types.size());
         query.header.options.set_response_code(Messages::Options::ResponseCode::NoError);
         query.header.options.set_recursion_desired(true);
@@ -327,21 +349,44 @@ public:
             });
         }
 
-        auto cached_entry = m_pending_lookups.with_write_locked([&](auto& pending_lookups) -> RefPtr<Core::Promise<NonnullRefPtr<LookupResult const>>> {
+        auto cached_entry = repeating_lookup ? nullptr : m_pending_lookups.with_write_locked([&](auto& pending_lookups) -> PendingLookup* {
             // One more try to make sure we're not overwriting an existing lookup
             if (cached_result_id.has_value()) {
                 if (auto* lookup = pending_lookups->find(*cached_result_id))
-                    return lookup->promise;
+                    return lookup;
             }
 
-            pending_lookups->insert(query.header.id, { query.header.id, name, result->make_weak_ptr(), promise });
+            pending_lookups->insert(query.header.id, { query.header.id, name, result->make_weak_ptr(), promise, Core::Timer::create(), 0 });
+            auto p = pending_lookups->find(query.header.id);
+            p->repeat_timer->set_single_shot(true);
+            p->repeat_timer->set_interval(1000);
+            p->repeat_timer->on_timeout = [=, this] {
+                (void)lookup(name, class_, desired_types, p);
+            };
+
             return nullptr;
         });
+
         if (cached_entry) {
-            dbgln_if(DNS_DEBUG, "DNS::lookup({}) -> Already in cache", name);
-            return cached_entry.release_nonnull();
+            dbgln_if(DNS_DEBUG, "DNS::lookup({}) -> Lookup already underway", name);
+            auto user_promise = Core::Promise<NonnullRefPtr<LookupResult const>>::construct();
+            promise->on_resolution = [user_promise, cached_promise = cached_entry->promise](auto& result) {
+                user_promise->resolve(*result);
+                cached_promise->resolve(*result);
+                return ErrorOr<void> {};
+            };
+            promise->on_rejection = [user_promise, cached_promise = cached_entry->promise](auto& error) {
+                user_promise->reject(Error::copy(error));
+                cached_promise->reject(Error::copy(error));
+            };
+            cached_entry->promise = move(promise);
+            return user_promise;
         }
 
+        auto pending_lookup = m_pending_lookups.with_write_locked([&](auto& lookups) -> PendingLookup* {
+            return lookups->find(query.header.id);
+        });
+
         ByteBuffer query_bytes;
         MUST(query.to_raw(query_bytes));
 
@@ -361,17 +406,12 @@ public:
             return promise;
         }
 
+        pending_lookup->repeat_timer->start();
+
         return promise;
     }
 
 private:
-    struct PendingLookup {
-        u16 id { 0 };
-        ByteString name;
-        WeakPtr<LookupResult> result;
-        NonnullRefPtr<Core::Promise<NonnullRefPtr<LookupResult const>>> promise;
-    };
-
     ErrorOr<Messages::Message> parse_one_message()
     {
         if (m_mode == ConnectionMode::UDP)
@@ -407,6 +447,7 @@ private:
                 if (!lookup)
                     return Error::from_string_literal("No pending lookup found for this message");
 
+                lookup->repeat_timer->stop();
                 if (lookup->result.is_null())
                     return {}; // Message is a response to a lookup that's been purged from the cache, ignore it
 

Services/RequestServer/ConnectionFromClient.cpp

@@ -344,120 +344,118 @@ void ConnectionFromClient::start_request(i32 request_id, ByteString const& metho
     }
 
     auto host = url.serialized_host().value().to_byte_string();
-    auto dns_promise = m_resolver->dns.lookup(host, DNS::Messages::Class::IN, Array { DNS::Messages::ResourceType::A, DNS::Messages::ResourceType::AAAA }.span());
-    auto resolve_result = dns_promise->await();
-    if (resolve_result.is_error()) {
-        dbgln("StartRequest: DNS lookup failed for '{}': {}", host, resolve_result.error());
-        async_request_finished(request_id, 0, Requests::NetworkError::UnableToResolveHost);
-        return;
-    }
-
-    auto dns_result = resolve_result.release_value();
-    if (dns_result->records().is_empty()) {
-        dbgln("StartRequest: DNS lookup failed for '{}'", host);
-        async_request_finished(request_id, 0, Requests::NetworkError::UnableToResolveHost);
-        return;
-    }
+    m_resolver->dns.lookup(host, DNS::Messages::Class::IN, Array { DNS::Messages::ResourceType::A, DNS::Messages::ResourceType::AAAA }.span())
+        ->when_rejected([this, request_id](auto const& error) {
+            dbgln("StartRequest: DNS lookup failed: {}", error);
+            async_request_finished(request_id, 0, Requests::NetworkError::UnableToResolveHost);
+        })
+        .when_resolved([this, request_id, host, url, method, request_body, request_headers, proxy_data](auto const& dns_result) {
+            if (dns_result->records().is_empty()) {
+                dbgln("StartRequest: DNS lookup failed for '{}'", host);
+                async_request_finished(request_id, 0, Requests::NetworkError::UnableToResolveHost);
+                return;
+            }
 
-    auto* easy = curl_easy_init();
-    if (!easy) {
-        dbgln("StartRequest: Failed to initialize curl easy handle");
-        return;
-    }
+            auto* easy = curl_easy_init();
+            if (!easy) {
+                dbgln("StartRequest: Failed to initialize curl easy handle");
+                return;
+            }
 
-    auto fds_or_error = Core::System::pipe2(O_NONBLOCK);
-    if (fds_or_error.is_error()) {
-        dbgln("StartRequest: Failed to create pipe: {}", fds_or_error.error());
-        return;
-    }
+            auto fds_or_error = Core::System::pipe2(O_NONBLOCK);
+            if (fds_or_error.is_error()) {
+                dbgln("StartRequest: Failed to create pipe: {}", fds_or_error.error());
+                return;
+            }
 
-    auto fds = fds_or_error.release_value();
-    auto writer_fd = fds[1];
-    auto reader_fd = fds[0];
-    async_request_started(request_id, IPC::File::adopt_fd(reader_fd));
+            auto fds = fds_or_error.release_value();
+            auto writer_fd = fds[1];
+            auto reader_fd = fds[0];
+            async_request_started(request_id, IPC::File::adopt_fd(reader_fd));
 
-    auto request = make<ActiveRequest>(*this, m_curl_multi, easy, request_id, writer_fd);
-    request->url = url.to_string().value();
+            auto request = make<ActiveRequest>(*this, m_curl_multi, easy, request_id, writer_fd);
+            request->url = url.to_string().value();
 
-    auto set_option = [easy](auto option, auto value) {
-        auto result = curl_easy_setopt(easy, option, value);
-        if (result != CURLE_OK) {
-            dbgln("StartRequest: Failed to set curl option: {}", curl_easy_strerror(result));
-            return false;
-        }
-        return true;
-    };
+            auto set_option = [easy](auto option, auto value) {
+                auto result = curl_easy_setopt(easy, option, value);
+                if (result != CURLE_OK) {
+                    dbgln("StartRequest: Failed to set curl option: {}", curl_easy_strerror(result));
+                    return false;
+                }
+                return true;
+            };
 
-    set_option(CURLOPT_PRIVATE, request.ptr());
+            set_option(CURLOPT_PRIVATE, request.ptr());
 
-    if (!g_default_certificate_path.is_empty())
-        set_option(CURLOPT_CAINFO, g_default_certificate_path.characters());
+            if (!g_default_certificate_path.is_empty())
+                set_option(CURLOPT_CAINFO, g_default_certificate_path.characters());
 
-    set_option(CURLOPT_ACCEPT_ENCODING, "gzip, deflate, br");
-    set_option(CURLOPT_URL, url.to_string().value().to_byte_string().characters());
-    set_option(CURLOPT_PORT, url.port_or_default());
-    set_option(CURLOPT_CONNECTTIMEOUT, s_connect_timeout_seconds);
+            set_option(CURLOPT_ACCEPT_ENCODING, "gzip, deflate, br");
+            set_option(CURLOPT_URL, url.to_string().value().to_byte_string().characters());
+            set_option(CURLOPT_PORT, url.port_or_default());
+            set_option(CURLOPT_CONNECTTIMEOUT, s_connect_timeout_seconds);
 
-    bool did_set_body = false;
+            bool did_set_body = false;
 
-    if (method == "GET"sv) {
-        set_option(CURLOPT_HTTPGET, 1L);
-    } else if (method.is_one_of("POST"sv, "PUT"sv, "PATCH"sv, "DELETE"sv)) {
-        request->body = request_body;
-        set_option(CURLOPT_POSTFIELDSIZE, request->body.size());
-        set_option(CURLOPT_POSTFIELDS, request->body.data());
-        did_set_body = true;
-    } else if (method == "HEAD") {
-        set_option(CURLOPT_NOBODY, 1L);
-    }
-    set_option(CURLOPT_CUSTOMREQUEST, method.characters());
+            if (method == "GET"sv) {
+                set_option(CURLOPT_HTTPGET, 1L);
+            } else if (method.is_one_of("POST"sv, "PUT"sv, "PATCH"sv, "DELETE"sv)) {
+                request->body = request_body;
+                set_option(CURLOPT_POSTFIELDSIZE, request->body.size());
+                set_option(CURLOPT_POSTFIELDS, request->body.data());
+                did_set_body = true;
+            } else if (method == "HEAD") {
+                set_option(CURLOPT_NOBODY, 1L);
+            }
+            set_option(CURLOPT_CUSTOMREQUEST, method.characters());
 
-    set_option(CURLOPT_FOLLOWLOCATION, 0);
+            set_option(CURLOPT_FOLLOWLOCATION, 0);
 
-    struct curl_slist* curl_headers = nullptr;
+            struct curl_slist* curl_headers = nullptr;
 
-    // NOTE: CURLOPT_POSTFIELDS automatically sets the Content-Type header.
-    //       Set it to empty if the headers passed in don't contain a content type.
-    if (did_set_body && !request_headers.contains("Content-Type"))
-        curl_headers = curl_slist_append(curl_headers, "Content-Type:");
+            // NOTE: CURLOPT_POSTFIELDS automatically sets the Content-Type header.
+            //       Set it to empty if the headers passed in don't contain a content type.
+            if (did_set_body && !request_headers.contains("Content-Type"))
+                curl_headers = curl_slist_append(curl_headers, "Content-Type:");
 
-    for (auto const& header : request_headers.headers()) {
-        auto header_string = ByteString::formatted("{}: {}", header.name, header.value);
-        curl_headers = curl_slist_append(curl_headers, header_string.characters());
-    }
-    set_option(CURLOPT_HTTPHEADER, curl_headers);
-
-    // FIXME: Set up proxy if applicable
-    (void)proxy_data;
-
-    set_option(CURLOPT_WRITEFUNCTION, &on_data_received);
-    set_option(CURLOPT_WRITEDATA, reinterpret_cast<void*>(request.ptr()));
-
-    set_option(CURLOPT_HEADERFUNCTION, &on_header_received);
-    set_option(CURLOPT_HEADERDATA, reinterpret_cast<void*>(request.ptr()));
-
-    StringBuilder resolve_opt_builder;
-    resolve_opt_builder.appendff("{}:{}:", host, url.port_or_default());
-    auto first = true;
-    for (auto& addr : dns_result->cached_addresses()) {
-        auto formatted_address = addr.visit(
-            [&](IPv4Address const& ipv4) { return ipv4.to_byte_string(); },
-            [&](IPv6Address const& ipv6) { return MUST(ipv6.to_string()).to_byte_string(); });
-        if (!first)
-            resolve_opt_builder.append(',');
-        first = false;
-        resolve_opt_builder.append(formatted_address);
-    }
+            for (auto const& header : request_headers.headers()) {
+                auto header_string = ByteString::formatted("{}: {}", header.name, header.value);
+                curl_headers = curl_slist_append(curl_headers, header_string.characters());
+            }
+            set_option(CURLOPT_HTTPHEADER, curl_headers);
+
+            // FIXME: Set up proxy if applicable
+            (void)proxy_data;
+
+            set_option(CURLOPT_WRITEFUNCTION, &on_data_received);
+            set_option(CURLOPT_WRITEDATA, reinterpret_cast<void*>(request.ptr()));
+
+            set_option(CURLOPT_HEADERFUNCTION, &on_header_received);
+            set_option(CURLOPT_HEADERDATA, reinterpret_cast<void*>(request.ptr()));
+
+            StringBuilder resolve_opt_builder;
+            resolve_opt_builder.appendff("{}:{}:", host, url.port_or_default());
+            auto first = true;
+            for (auto& addr : dns_result->cached_addresses()) {
+                auto formatted_address = addr.visit(
+                    [&](IPv4Address const& ipv4) { return ipv4.to_byte_string(); },
+                    [&](IPv6Address const& ipv6) { return MUST(ipv6.to_string()).to_byte_string(); });
+                if (!first)
+                    resolve_opt_builder.append(',');
+                first = false;
+                resolve_opt_builder.append(formatted_address);
+            }
 
-    auto formatted_address = resolve_opt_builder.to_byte_string();
-    g_dns_cache.set(host, formatted_address);
-    curl_slist* resolve_list = curl_slist_append(nullptr, formatted_address.characters());
-    curl_easy_setopt(easy, CURLOPT_RESOLVE, resolve_list);
+            auto formatted_address = resolve_opt_builder.to_byte_string();
+            g_dns_cache.set(host, formatted_address);
+            curl_slist* resolve_list = curl_slist_append(nullptr, formatted_address.characters());
+            curl_easy_setopt(easy, CURLOPT_RESOLVE, resolve_list);
 
-    auto result = curl_multi_add_handle(m_curl_multi, easy);
-    VERIFY(result == CURLM_OK);
+            auto result = curl_multi_add_handle(m_curl_multi, easy);
+            VERIFY(result == CURLM_OK);
 
-    m_active_requests.set(request_id, move(request));
+            m_active_requests.set(request_id, move(request));
+        });
 }
 
 static Requests::NetworkError map_curl_code_to_network_error(CURLcode const& code)