RequestServer: Create disk cache writers for new requests immediately

We previously waited until we received all response headers before we
would create the cache entry. We now create one immediately, and handle
writing the headers in its own function. This will allow us to know if
a cache entry writer already exists for a given cache key, and thus
prevent creating a second writer at the same time.

Author: trflynn89

Services/RequestServer/Cache/CacheEntry.cpp

@@ -90,16 +90,49 @@ void CacheEntry::close_and_destroy_cache_entry()
     m_disk_cache.cache_entry_closed({}, *this);
 }
 
-ErrorOr<NonnullOwnPtr<CacheEntryWriter>> CacheEntryWriter::create(DiskCache& disk_cache, CacheIndex& index, u64 cache_key, String url, u32 status_code, Optional<String> reason_phrase, HTTP::HeaderMap const& headers, UnixDateTime request_time)
+ErrorOr<NonnullOwnPtr<CacheEntryWriter>> CacheEntryWriter::create(DiskCache& disk_cache, CacheIndex& index, u64 cache_key, String url, UnixDateTime request_time)
 {
     auto path = path_for_cache_key(disk_cache.cache_directory(), cache_key);
 
     auto unbuffered_file = TRY(Core::File::open(path.string(), Core::File::OpenMode::Write));
     auto file = TRY(Core::OutputBufferedFile::create(move(unbuffered_file)));
 
     CacheHeader cache_header;
+    cache_header.url_size = url.byte_count();
+    cache_header.url_hash = url.hash();
+
+    return adopt_own(*new CacheEntryWriter { disk_cache, index, cache_key, move(url), move(path), move(file), cache_header, request_time });
+}
+
+CacheEntryWriter::CacheEntryWriter(DiskCache& disk_cache, CacheIndex& index, u64 cache_key, String url, LexicalPath path, NonnullOwnPtr<Core::OutputBufferedFile> file, CacheHeader cache_header, UnixDateTime request_time)
+    : CacheEntry(disk_cache, index, cache_key, move(url), move(path), cache_header)
+    , m_file(move(file))
+    , m_request_time(request_time)
+    , m_response_time(UnixDateTime::now())
+{
+}
+
+ErrorOr<void> CacheEntryWriter::write_headers(u32 status_code, Optional<String> reason_phrase, HTTP::HeaderMap const& headers)
+{
+    if (m_marked_for_deletion) {
+        close_and_destroy_cache_entry();
+        return Error::from_string_literal("Cache entry has been deleted");
+    }
+
+    m_cache_header.status_code = status_code;
+
+    if (reason_phrase.has_value()) {
+        m_cache_header.reason_phrase_size = reason_phrase->byte_count();
+        m_cache_header.reason_phrase_hash = reason_phrase->hash();
+    }
 
     auto result = [&]() -> ErrorOr<void> {
+        if (!is_cacheable(status_code, headers))
+            return Error::from_string_literal("Response is not cacheable");
+
+        if (auto freshness = calculate_freshness_lifetime(headers); freshness.is_negative() || freshness.is_zero())
+            return Error::from_string_literal("Response has already expired");
+
         StringBuilder builder;
         auto headers_serializer = TRY(JsonArraySerializer<>::try_create(builder));
 
@@ -114,41 +147,29 @@ ErrorOr<NonnullOwnPtr<CacheEntryWriter>> CacheEntryWriter::create(DiskCache& dis
         }
 
         TRY(headers_serializer.finish());
-
-        cache_header.url_size = url.byte_count();
-        cache_header.url_hash = url.hash();
-
-        cache_header.status_code = status_code;
-        cache_header.reason_phrase_size = reason_phrase.has_value() ? reason_phrase->byte_count() : 0;
-        cache_header.reason_phrase_hash = reason_phrase.has_value() ? reason_phrase->hash() : 0;
-
         auto serialized_headers = builder.string_view();
-        cache_header.headers_size = serialized_headers.length();
-        cache_header.headers_hash = serialized_headers.hash();
+        m_cache_header.headers_size = serialized_headers.length();
+        m_cache_header.headers_hash = serialized_headers.hash();
 
-        TRY(file->write_value(cache_header));
-        TRY(file->write_until_depleted(url));
+        TRY(m_file->write_value(m_cache_header));
+        TRY(m_file->write_until_depleted(m_url));
         if (reason_phrase.has_value())
-            TRY(file->write_until_depleted(*reason_phrase));
-        TRY(file->write_until_depleted(serialized_headers));
+            TRY(m_file->write_until_depleted(*reason_phrase));
+        TRY(m_file->write_until_depleted(serialized_headers));
 
         return {};
     }();
 
     if (result.is_error()) {
-        (void)FileSystem::remove(path.string(), FileSystem::RecursionMode::Disallowed);
+        dbgln("\033[31;1mUnable to write headers to cache entry for\033[0m {}: {}", m_url, result.error());
+
+        remove();
+        close_and_destroy_cache_entry();
+
         return result.release_error();
     }
 
-    return adopt_own(*new CacheEntryWriter { disk_cache, index, cache_key, move(url), path, move(file), cache_header, request_time });
-}
-
-CacheEntryWriter::CacheEntryWriter(DiskCache& disk_cache, CacheIndex& index, u64 cache_key, String url, LexicalPath path, NonnullOwnPtr<Core::OutputBufferedFile> file, CacheHeader cache_header, UnixDateTime request_time)
-    : CacheEntry(disk_cache, index, cache_key, move(url), move(path), cache_header)
-    , m_file(move(file))
-    , m_request_time(request_time)
-    , m_response_time(UnixDateTime::now())
-{
+    return {};
 }
 
 ErrorOr<void> CacheEntryWriter::write_data(ReadonlyBytes data)
@@ -159,7 +180,7 @@ ErrorOr<void> CacheEntryWriter::write_data(ReadonlyBytes data)
     }
 
     if (auto result = m_file->write_until_depleted(data); result.is_error()) {
-        dbgln("\033[31;1mUnable to write to cache entry for\033[0m {}: {}", m_url, result.error());
+        dbgln("\033[31;1mUnable to write data to cache entry for\033[0m {}: {}", m_url, result.error());
 
         remove();
         close_and_destroy_cache_entry();

Services/RequestServer/Cache/CacheEntry.h

@@ -80,9 +80,10 @@ class CacheEntry {
 
 class CacheEntryWriter : public CacheEntry {
 public:
-    static ErrorOr<NonnullOwnPtr<CacheEntryWriter>> create(DiskCache&, CacheIndex&, u64 cache_key, String url, u32 status_code, Optional<String> reason_phrase, HTTP::HeaderMap const&, UnixDateTime request_time);
+    static ErrorOr<NonnullOwnPtr<CacheEntryWriter>> create(DiskCache&, CacheIndex&, u64 cache_key, String url, UnixDateTime request_time);
     virtual ~CacheEntryWriter() override = default;
 
+    ErrorOr<void> write_headers(u32 status_code, Optional<String> reason_phrase, HTTP::HeaderMap const&);
     ErrorOr<void> write_data(ReadonlyBytes);
     ErrorOr<void> flush();
 

Services/RequestServer/Cache/DiskCache.cpp

@@ -32,18 +32,15 @@ DiskCache::DiskCache(NonnullRefPtr<Database::Database> database, LexicalPath cac
 {
 }
 
-Optional<CacheEntryWriter&> DiskCache::create_entry(URL::URL const& url, StringView method, u32 status_code, Optional<String> reason_phrase, HTTP::HeaderMap const& headers, UnixDateTime request_time)
+Optional<CacheEntryWriter&> DiskCache::create_entry(URL::URL const& url, StringView method, UnixDateTime request_time)
 {
-    if (!is_cacheable(method, status_code, headers))
-        return {};
-
-    if (auto freshness = calculate_freshness_lifetime(headers); freshness.is_negative() || freshness.is_zero())
+    if (!is_cacheable(method))
         return {};
 
     auto serialized_url = serialize_url_for_cache_storage(url);
     auto cache_key = create_cache_key(serialized_url, method);
 
-    auto cache_entry = CacheEntryWriter::create(*this, m_index, cache_key, move(serialized_url), status_code, move(reason_phrase), headers, request_time);
+    auto cache_entry = CacheEntryWriter::create(*this, m_index, cache_key, move(serialized_url), request_time);
     if (cache_entry.is_error()) {
         dbgln("\033[31;1mUnable to create cache entry for\033[0m {}: {}", url, cache_entry.error());
         return {};
@@ -59,6 +56,9 @@ Optional<CacheEntryWriter&> DiskCache::create_entry(URL::URL const& url, StringV
 
 Optional<CacheEntryReader&> DiskCache::open_entry(URL::URL const& url, StringView method)
 {
+    if (!is_cacheable(method))
+        return {};
+
     auto serialized_url = serialize_url_for_cache_storage(url);
     auto cache_key = create_cache_key(serialized_url, method);
 

Services/RequestServer/Cache/DiskCache.h

@@ -13,7 +13,6 @@
 #include <AK/Time.h>
 #include <AK/Types.h>
 #include <LibDatabase/Database.h>
-#include <LibHTTP/HeaderMap.h>
 #include <LibURL/Forward.h>
 #include <RequestServer/Cache/CacheEntry.h>
 #include <RequestServer/Cache/CacheIndex.h>
@@ -24,7 +23,7 @@ class DiskCache {
 public:
     static ErrorOr<DiskCache> create();
 
-    Optional<CacheEntryWriter&> create_entry(URL::URL const&, StringView method, u32 status_code, Optional<String> reason_phrase, HTTP::HeaderMap const&, UnixDateTime request_time);
+    Optional<CacheEntryWriter&> create_entry(URL::URL const&, StringView method, UnixDateTime request_time);
     Optional<CacheEntryReader&> open_entry(URL::URL const&, StringView method);
     void clear_cache();
 

Services/RequestServer/Cache/Utilities.cpp

@@ -71,13 +71,18 @@ u64 create_cache_key(StringView url, StringView method)
 }
 
 // https://httpwg.org/specs/rfc9111.html#response.cacheability
-bool is_cacheable(StringView method, u32 status_code, HTTP::HeaderMap const& headers)
+bool is_cacheable(StringView method)
 {
     // A cache MUST NOT store a response to a request unless:
 
     // * the request method is understood by the cache;
-    if (!method.is_one_of("GET"sv, "HEAD"sv))
-        return false;
+    return method.is_one_of("GET"sv, "HEAD"sv);
+}
+
+// https://httpwg.org/specs/rfc9111.html#response.cacheability
+bool is_cacheable(u32 status_code, HTTP::HeaderMap const& headers)
+{
+    // A cache MUST NOT store a response to a request unless:
 
     // * the response status code is final (see Section 15 of [HTTP]);
     if (status_code < 200)

Services/RequestServer/Cache/Utilities.h

@@ -17,7 +17,8 @@ namespace RequestServer {
 String serialize_url_for_cache_storage(URL::URL const&);
 u64 create_cache_key(StringView url, StringView method);
 
-bool is_cacheable(StringView method, u32 status_code, HTTP::HeaderMap const&);
+bool is_cacheable(StringView method);
+bool is_cacheable(u32 status_code, HTTP::HeaderMap const&);
 bool is_header_exempted_from_storage(StringView name);
 
 AK::Duration calculate_freshness_lifetime(HTTP::HeaderMap const&);

Services/RequestServer/Request.cpp

@@ -172,6 +172,8 @@ void Request::handle_initial_state()
             transition_to_state(State::ReadCache);
             return;
         }
+
+        m_cache_entry_writer = m_disk_cache->create_entry(m_url, m_method, m_request_start_time);
     }
 
     transition_to_state(State::DNSLookup);
@@ -486,8 +488,10 @@ void Request::transfer_headers_to_client_if_needed()
     m_status_code = acquire_status_code();
     m_client.async_headers_became_available(m_request_id, m_response_headers, m_status_code, m_reason_phrase);
 
-    if (m_disk_cache.has_value())
-        m_cache_entry_writer = m_disk_cache->create_entry(m_url, m_method, m_status_code, m_reason_phrase, m_response_headers, m_request_start_time);
+    if (m_cache_entry_writer.has_value()) {
+        if (m_cache_entry_writer->write_headers(m_status_code, m_reason_phrase, m_response_headers).is_error())
+            m_cache_entry_writer.clear();
+    }
 }
 
 ErrorOr<void> Request::write_queued_bytes_without_blocking()