From 4211639e455c8f58f23304be5312ad48a76e401a Mon Sep 17 00:00:00 2001 From: Ali Mohammad Pur Date: Sun, 28 Jul 2024 00:22:58 +0200 Subject: [PATCH] RequestServer+LibHTTP: Cancel requests on client exit That usually happens in "exceptional" states when the client exits unexpectedly (crash, force quit mid-load, etc), leading to the job flush timer firing and attempting to write to a nonexistent object (the client). This commit makes RS simply cancel such jobs; cancelled jobs in this state simply go away instead of sending notifications around. --- Userland/Libraries/LibHTTP/Job.cpp | 7 +++++++ Userland/Services/RequestServer/ConnectionCache.h | 2 +- .../Services/RequestServer/ConnectionFromClient.cpp | 13 ++++++++++--- .../Services/RequestServer/ConnectionFromClient.h | 2 +- Userland/Services/RequestServer/HttpCommon.h | 4 ++-- Userland/Services/RequestServer/HttpRequest.h | 1 + Userland/Services/RequestServer/HttpsRequest.h | 1 + Userland/Services/RequestServer/Request.h | 2 ++ 8 files changed, 25 insertions(+), 7 deletions(-) diff --git a/Userland/Libraries/LibHTTP/Job.cpp b/Userland/Libraries/LibHTTP/Job.cpp index 2e1074189d2..2d9a60e5c6c 100644 --- a/Userland/Libraries/LibHTTP/Job.cpp +++ b/Userland/Libraries/LibHTTP/Job.cpp @@ -576,6 +576,13 @@ void Job::finish_up() { VERIFY(!m_has_scheduled_finish); m_state = State::Finished; + + if (is_cancelled()) { + stop_timer(); + m_has_scheduled_finish = true; + return; + } + if (!m_can_stream_response) { auto maybe_flattened_buffer = ByteBuffer::create_uninitialized(m_buffered_size); if (maybe_flattened_buffer.is_error()) diff --git a/Userland/Services/RequestServer/ConnectionCache.h b/Userland/Services/RequestServer/ConnectionCache.h index 3b4957c2603..ecb5389c5c9 100644 --- a/Userland/Services/RequestServer/ConnectionCache.h +++ b/Userland/Services/RequestServer/ConnectionCache.h @@ -101,7 +101,7 @@ struct JobData { #endif template - static JobData create(NonnullRefPtr job, [[maybe_unused]] URL::URL url) + static JobData create(WeakPtr job, [[maybe_unused]] URL::URL url) { return JobData { [job](auto& socket) { job->start(socket); }, diff --git a/Userland/Services/RequestServer/ConnectionFromClient.cpp b/Userland/Services/RequestServer/ConnectionFromClient.cpp index 8bb1d22f339..bc562b24c16 100644 --- a/Userland/Services/RequestServer/ConnectionFromClient.cpp +++ b/Userland/Services/RequestServer/ConnectionFromClient.cpp @@ -46,6 +46,14 @@ ConnectionFromClient::ConnectionFromClient(NonnullOwnPtr sock s_connections.set(client_id(), *this); } +ConnectionFromClient::~ConnectionFromClient() +{ + m_requests.with_locked([](HashMap>& map) { + for (auto& entry : map) + entry.value->cancel(); + }); +} + class Job : public RefCounted , public Weakable { public: @@ -152,10 +160,9 @@ void ConnectionFromClient::worker_do_work(Work work) return; } - auto job = Job::ensure(url); dbgln("EnsureConnection: Pre-connect to {}", url); - auto do_preconnect = [&](auto& cache) { - ConnectionCache::ensure_connection(cache, url, job); + auto do_preconnect = [=, job = Job::ensure(url)](auto& cache) { + ConnectionCache::ensure_connection(cache, url, job->make_weak_ptr()); }; if (url.scheme() == "http"sv) diff --git a/Userland/Services/RequestServer/ConnectionFromClient.h b/Userland/Services/RequestServer/ConnectionFromClient.h index f8e5d9f73bf..95ceafd142f 100644 --- a/Userland/Services/RequestServer/ConnectionFromClient.h +++ b/Userland/Services/RequestServer/ConnectionFromClient.h @@ -23,7 +23,7 @@ class ConnectionFromClient final C_OBJECT(ConnectionFromClient); public: - ~ConnectionFromClient() override = default; + ~ConnectionFromClient() override; virtual void die() override; diff --git a/Userland/Services/RequestServer/HttpCommon.h b/Userland/Services/RequestServer/HttpCommon.h index 89388b1d958..3cff2308971 100644 --- a/Userland/Services/RequestServer/HttpCommon.h +++ b/Userland/Services/RequestServer/HttpCommon.h @@ -99,10 +99,10 @@ OwnPtr start_request(TBadgedProtocol&& protocol, i32 request_id, Connec auto output_stream = MUST(Core::File::adopt_fd(pipe_result.value().write_fd, Core::File::OpenMode::Write)); auto job = TJob::construct(move(request), *output_stream); - auto protocol_request = TRequest::create_with_job(forward(protocol), client, (TJob&)*job, move(output_stream), request_id); + auto protocol_request = TRequest::create_with_job(forward(protocol), client, static_cast(*job), move(output_stream), request_id); protocol_request->set_request_fd(pipe_result.value().read_fd); - Core::deferred_invoke([=] { + Core::deferred_invoke([=, job = job->template make_weak_ptr()] { if constexpr (IsSame) ConnectionCache::ensure_connection(ConnectionCache::g_tls_connection_cache, url, job, proxy_data); else diff --git a/Userland/Services/RequestServer/HttpRequest.h b/Userland/Services/RequestServer/HttpRequest.h index 1f25d9dd7cd..ea18b599c53 100644 --- a/Userland/Services/RequestServer/HttpRequest.h +++ b/Userland/Services/RequestServer/HttpRequest.h @@ -23,6 +23,7 @@ public: HTTP::Job const& job() const { return m_job; } virtual URL::URL url() const override { return m_job->url(); } + virtual void cancel() override { m_job->cancel(); } private: explicit HttpRequest(ConnectionFromClient&, NonnullRefPtr, NonnullOwnPtr&&, i32); diff --git a/Userland/Services/RequestServer/HttpsRequest.h b/Userland/Services/RequestServer/HttpsRequest.h index 146d1d8a8db..1eb7e7097bb 100644 --- a/Userland/Services/RequestServer/HttpsRequest.h +++ b/Userland/Services/RequestServer/HttpsRequest.h @@ -22,6 +22,7 @@ public: HTTP::HttpsJob const& job() const { return m_job; } virtual URL::URL url() const override { return m_job->url(); } + virtual void cancel() override { m_job->cancel(); } private: explicit HttpsRequest(ConnectionFromClient&, NonnullRefPtr, NonnullOwnPtr&&, i32); diff --git a/Userland/Services/RequestServer/Request.h b/Userland/Services/RequestServer/Request.h index 31ec9023f67..4614ba8b3a3 100644 --- a/Userland/Services/RequestServer/Request.h +++ b/Userland/Services/RequestServer/Request.h @@ -34,6 +34,8 @@ public: void set_request_fd(int fd) { m_request_fd = fd; } int request_fd() const { return m_request_fd; } + virtual void cancel() = 0; + void did_finish(bool success); void did_progress(Optional total_size, u64 downloaded_size); void set_status_code(u32 status_code) { m_status_code = status_code; }