RGBCube/serenity
1
Fork 0
mirror of https://github.com/RGBCube/serenity synced 2025-07-25 18:27:35 +00:00
Code Issues Activity Actions

FileSystemAccessServer: Manage concurrent file requests

Browse source 
Before this patch, when you called FileSystemAccessServer::Client::try_*
twice, the second call used the same variable to store the promise. This
"race condition" is now solved using a HashMap, to store multiple
parallel requests.
This commit is contained in:
Lucas CHOLLET 2022-02-26 17:24:22 +01:00 • committed by Linus Groh
parent e432a284d7
commit 9521952f36
6 changed files with 88 additions and 61 deletions
Download patch file Download diff file Expand all files Collapse all files

80
Userland/Libraries/LibFileSystemAccessClient/Client.cpp
View file

@ -28,8 +28,8 @@ Client& Client::the()
Result Client::try_request_file_read_only_approved(GUI::Window* parent_window, String const& path) Result Client::try_request_file_read_only_approved(GUI::Window* parent_window, String const& path)
{ {
m_promise = Core::Promise<Result>::construct(); auto const id = get_new_id();
m_parent_window = parent_window; m_promises.set(id, PromiseAndWindow { Core::Promise<Result>::construct(), parent_window });
auto parent_window_server_client_id = GUI::ConnectionToWindowServer::the().expose_client_id(); auto parent_window_server_client_id = GUI::ConnectionToWindowServer::the().expose_client_id();
auto child_window_server_client_id = expose_window_server_client_id(); auto child_window_server_client_id = expose_window_server_client_id();
@ -42,19 +42,19 @@ Result Client::try_request_file_read_only_approved(GUI::Window* parent_window, S
}); });
if (path.starts_with('/')) { if (path.starts_with('/')) {
async_request_file_read_only_approved(parent_window_server_client_id, parent_window_id, path); async_request_file_read_only_approved(id, parent_window_server_client_id, parent_window_id, path);
} else { } else {
auto full_path = LexicalPath::join(Core::File::current_working_directory(), path).string(); auto full_path = LexicalPath::join(Core::File::current_working_directory(), path).string();
async_request_file_read_only_approved(parent_window_server_client_id, parent_window_id, full_path); async_request_file_read_only_approved(id, parent_window_server_client_id, parent_window_id, full_path);
} }
return m_promise->await(); return handle_promise(id);
} }
Result Client::try_request_file(GUI::Window* parent_window, String const& path, Core::OpenMode mode) Result Client::try_request_file(GUI::Window* parent_window, String const& path, Core::OpenMode mode)
{ {
m_promise = Core::Promise<Result>::construct(); auto const id = get_new_id();
m_parent_window = parent_window; m_promises.set(id, PromiseAndWindow { Core::Promise<Result>::construct(), parent_window });
auto parent_window_server_client_id = GUI::ConnectionToWindowServer::the().expose_client_id(); auto parent_window_server_client_id = GUI::ConnectionToWindowServer::the().expose_client_id();
auto child_window_server_client_id = expose_window_server_client_id(); auto child_window_server_client_id = expose_window_server_client_id();
@ -67,19 +67,19 @@ Result Client::try_request_file(GUI::Window* parent_window, String const& path,
}); });
if (path.starts_with('/')) { if (path.starts_with('/')) {
async_request_file(parent_window_server_client_id, parent_window_id, path, mode); async_request_file(id, parent_window_server_client_id, parent_window_id, path, mode);
} else { } else {
auto full_path = LexicalPath::join(Core::File::current_working_directory(), path).string(); auto full_path = LexicalPath::join(Core::File::current_working_directory(), path).string();
async_request_file(parent_window_server_client_id, parent_window_id, full_path, mode); async_request_file(id, parent_window_server_client_id, parent_window_id, full_path, mode);
} }
return m_promise->await(); return handle_promise(id);
} }
Result Client::try_open_file(GUI::Window* parent_window, String const& window_title, StringView path, Core::OpenMode requested_access) Result Client::try_open_file(GUI::Window* parent_window, String const& window_title, StringView path, Core::OpenMode requested_access)
{ {
m_promise = Core::Promise<Result>::construct(); auto const id = get_new_id();
m_parent_window = parent_window; m_promises.set(id, PromiseAndWindow { Core::Promise<Result>::construct(), parent_window });
auto parent_window_server_client_id = GUI::ConnectionToWindowServer::the().expose_client_id(); auto parent_window_server_client_id = GUI::ConnectionToWindowServer::the().expose_client_id();
auto child_window_server_client_id = expose_window_server_client_id(); auto child_window_server_client_id = expose_window_server_client_id();
@ -91,15 +91,15 @@ Result Client::try_open_file(GUI::Window* parent_window, String const& window_ti
GUI::ConnectionToWindowServer::the().async_remove_window_stealing_for_client(child_window_server_client_id, parent_window_id); GUI::ConnectionToWindowServer::the().async_remove_window_stealing_for_client(child_window_server_client_id, parent_window_id);
}); });
async_prompt_open_file(parent_window_server_client_id, parent_window_id, window_title, path, requested_access); async_prompt_open_file(id, parent_window_server_client_id, parent_window_id, window_title, path, requested_access);
return m_promise->await(); return handle_promise(id);
} }
Result Client::try_save_file(GUI::Window* parent_window, String const& name, String const ext, Core::OpenMode requested_access) Result Client::try_save_file(GUI::Window* parent_window, String const& name, String const ext, Core::OpenMode requested_access)
{ {
m_promise = Core::Promise<Result>::construct(); auto const id = get_new_id();
m_parent_window = parent_window; m_promises.set(id, PromiseAndWindow { Core::Promise<Result>::construct(), parent_window });
auto parent_window_server_client_id = GUI::ConnectionToWindowServer::the().expose_client_id(); auto parent_window_server_client_id = GUI::ConnectionToWindowServer::the().expose_client_id();
auto child_window_server_client_id = expose_window_server_client_id(); auto child_window_server_client_id = expose_window_server_client_id();
@ -111,51 +111,69 @@ Result Client::try_save_file(GUI::Window* parent_window, String const& name, Str
GUI::ConnectionToWindowServer::the().async_remove_window_stealing_for_client(child_window_server_client_id, parent_window_id); GUI::ConnectionToWindowServer::the().async_remove_window_stealing_for_client(child_window_server_client_id, parent_window_id);
}); });
async_prompt_save_file(parent_window_server_client_id, parent_window_id, name.is_null() ? "Untitled" : name, ext.is_null() ? "txt" : ext, Core::StandardPaths::home_directory(), requested_access); async_prompt_save_file(id, parent_window_server_client_id, parent_window_id, name.is_null() ? "Untitled" : name, ext.is_null() ? "txt" : ext, Core::StandardPaths::home_directory(), requested_access);
return m_promise->await(); return handle_promise(id);
} }
void Client::handle_prompt_end(i32 error, Optional<IPC::File> const& ipc_file, Optional<String> const& chosen_file) void Client::handle_prompt_end(i32 request_id, i32 error, Optional<IPC::File> const& ipc_file, Optional<String> const& chosen_file)
{ {
VERIFY(m_promise); auto potential_data = m_promises.get(request_id);
VERIFY(potential_data.has_value());
auto& request_data = potential_data.value();
if (error != 0) { if (error != 0) {
// We don't want to show an error message for non-existent files since some applications may want // We don't want to show an error message for non-existent files since some applications may want
// to handle it as opening a new, named file. // to handle it as opening a new, named file.
if (error != -1 && error != ENOENT) if (error != -1 && error != ENOENT)
GUI::MessageBox::show_error(m_parent_window, String::formatted("Opening \"{}\" failed: {}", *chosen_file, strerror(error))); GUI::MessageBox::show_error(request_data.parent_window, String::formatted("Opening \"{}\" failed: {}", *chosen_file, strerror(error)));
m_promise->resolve(Error::from_errno(error)); request_data.promise->resolve(Error::from_errno(error));
return; return;
} }
auto file = Core::File::construct(); auto file = Core::File::construct();
auto fd = ipc_file->take_fd(); auto fd = ipc_file->take_fd();
if (!file->open(fd, Core::OpenMode::ReadWrite, Core::File::ShouldCloseFileDescriptor::Yes) && file->error() != ENOENT) { if (!file->open(fd, Core::OpenMode::ReadWrite, Core::File::ShouldCloseFileDescriptor::Yes) && file->error() != ENOENT) {
GUI::MessageBox::show_error(m_parent_window, String::formatted("Opening \"{}\" failed: {}", *chosen_file, strerror(error))); GUI::MessageBox::show_error(request_data.parent_window, String::formatted("Opening \"{}\" failed: {}", *chosen_file, strerror(error)));
m_promise->resolve(Error::from_errno(file->error())); request_data.promise->resolve(Error::from_errno(file->error()));
return; return;
} }
if (file->is_device()) { if (file->is_device()) {
GUI::MessageBox::show_error(m_parent_window, String::formatted("Opening \"{}\" failed: Cannot open device files", *chosen_file)); GUI::MessageBox::show_error(request_data.parent_window, String::formatted("Opening \"{}\" failed: Cannot open device files", *chosen_file));
m_promise->resolve(Error::from_string_literal("Cannot open device files"sv)); request_data.promise->resolve(Error::from_string_literal("Cannot open device files"sv));
return; return;
} }
if (file->is_directory()) { if (file->is_directory()) {
GUI::MessageBox::show_error(m_parent_window, String::formatted("Opening \"{}\" failed: Cannot open directory", *chosen_file)); GUI::MessageBox::show_error(request_data.parent_window, String::formatted("Opening \"{}\" failed: Cannot open directory", *chosen_file));
m_promise->resolve(Error::from_string_literal("Cannot open directory"sv)); request_data.promise->resolve(Error::from_string_literal("Cannot open directory"sv));
return; return;
} }
file->set_filename(move(*chosen_file)); file->set_filename(move(*chosen_file));
m_promise->resolve(file); request_data.promise->resolve(file);
} }
void Client::die() void Client::die()
{ {
if (m_promise) for (auto const& entry : m_promises)
handle_prompt_end(ECONNRESET, {}, ""); handle_prompt_end(entry.key, ECONNRESET, {}, "");
}
int Client::get_new_id()
{
auto const new_id = m_last_id++;
// Note: This verify shouldn't fail, and we should provide a valid ID
// But we probably have more issues if this test fails.
VERIFY(!m_promises.contains(new_id));
return new_id;
}
Result Client::handle_promise(int id)
{
auto result = m_promises.get(id)->promise->await();
m_promises.remove(id);
return result;
} }
} }

15
Userland/Libraries/LibFileSystemAccessClient/Client.h
View file

@ -7,6 +7,7 @@
#pragma once #pragma once
#include <AK/HashMap.h>
#include <FileSystemAccessServer/FileSystemAccessClientEndpoint.h> #include <FileSystemAccessServer/FileSystemAccessClientEndpoint.h>
#include <FileSystemAccessServer/FileSystemAccessServerEndpoint.h> #include <FileSystemAccessServer/FileSystemAccessServerEndpoint.h>
#include <LibCore/File.h> #include <LibCore/File.h>
@ -41,10 +42,18 @@ private:
{ {
} }
virtual void handle_prompt_end(i32 error, Optional<IPC::File> const& fd, Optional<String> const& chosen_file) override; virtual void handle_prompt_end(i32 request_id, i32 error, Optional<IPC::File> const& fd, Optional<String> const& chosen_file) override;
RefPtr<Core::Promise<Result>> m_promise; int get_new_id();
GUI::Window* m_parent_window { nullptr }; Result handle_promise(int);
struct PromiseAndWindow {
RefPtr<Core::Promise<Result>> promise {};
GUI::Window* parent_window { nullptr };
};
HashMap<int, PromiseAndWindow> m_promises {};
int m_last_id { 0 };
}; };
} }

32
Userland/Services/FileSystemAccessServer/ConnectionFromClient.cpp
View file

@ -45,7 +45,7 @@ RefPtr<GUI::Window> ConnectionFromClient::create_dummy_child_window(i32 window_s
return window; return window;
} }
void ConnectionFromClient::request_file_handler(i32 window_server_client_id, i32 parent_window_id, String const& path, Core::OpenMode const& requested_access, ShouldPrompt prompt) void ConnectionFromClient::request_file_handler(i32 request_id, i32 window_server_client_id, i32 parent_window_id, String const& path, Core::OpenMode const& requested_access, ShouldPrompt prompt)
{ {
VERIFY(path.starts_with("/"sv)); VERIFY(path.starts_with("/"sv));
@ -97,26 +97,26 @@ void ConnectionFromClient::request_file_handler(i32 window_server_client_id, i32
if (file.is_error()) { if (file.is_error()) {
dbgln("FileSystemAccessServer: Couldn't open {}, error {}", path, file.error()); dbgln("FileSystemAccessServer: Couldn't open {}, error {}", path, file.error());
async_handle_prompt_end(errno, Optional<IPC::File> {}, path); async_handle_prompt_end(request_id, errno, Optional<IPC::File> {}, path);
} else { } else {
async_handle_prompt_end(0, IPC::File(file.value()->leak_fd(), IPC::File::CloseAfterSending), path); async_handle_prompt_end(request_id, 0, IPC::File(file.value()->leak_fd(), IPC::File::CloseAfterSending), path);
} }
} else { } else {
async_handle_prompt_end(-1, Optional<IPC::File> {}, path); async_handle_prompt_end(request_id, -1, Optional<IPC::File> {}, path);
} }
} }
void ConnectionFromClient::request_file_read_only_approved(i32 window_server_client_id, i32 parent_window_id, String const& path) void ConnectionFromClient::request_file_read_only_approved(i32 request_id, i32 window_server_client_id, i32 parent_window_id, String const& path)
{ {
request_file_handler(window_server_client_id, parent_window_id, path, Core::OpenMode::ReadOnly, ShouldPrompt::No); request_file_handler(request_id, window_server_client_id, parent_window_id, path, Core::OpenMode::ReadOnly, ShouldPrompt::No);
} }
void ConnectionFromClient::request_file(i32 window_server_client_id, i32 parent_window_id, String const& path, Core::OpenMode const& requested_access) void ConnectionFromClient::request_file(i32 request_id, i32 window_server_client_id, i32 parent_window_id, String const& path, Core::OpenMode const& requested_access)
{ {
request_file_handler(window_server_client_id, parent_window_id, path, requested_access, ShouldPrompt::Yes); request_file_handler(request_id, window_server_client_id, parent_window_id, path, requested_access, ShouldPrompt::Yes);
} }
void ConnectionFromClient::prompt_open_file(i32 window_server_client_id, i32 parent_window_id, String const& window_title, String const& path_to_view, Core::OpenMode const& requested_access) void ConnectionFromClient::prompt_open_file(i32 request_id, i32 window_server_client_id, i32 parent_window_id, String const& window_title, String const& path_to_view, Core::OpenMode const& requested_access)
{ {
auto relevant_permissions = requested_access & (Core::OpenMode::ReadOnly | Core::OpenMode::WriteOnly); auto relevant_permissions = requested_access & (Core::OpenMode::ReadOnly | Core::OpenMode::WriteOnly);
VERIFY(relevant_permissions != Core::OpenMode::NotOpen); VERIFY(relevant_permissions != Core::OpenMode::NotOpen);
@ -125,10 +125,10 @@ void ConnectionFromClient::prompt_open_file(i32 window_server_client_id, i32 par
auto user_picked_file = GUI::FilePicker::get_open_filepath(main_window, window_title, path_to_view); auto user_picked_file = GUI::FilePicker::get_open_filepath(main_window, window_title, path_to_view);
prompt_helper(user_picked_file, requested_access); prompt_helper(request_id, user_picked_file, requested_access);
} }
void ConnectionFromClient::prompt_save_file(i32 window_server_client_id, i32 parent_window_id, String const& name, String const& ext, String const& path_to_view, Core::OpenMode const& requested_access) void ConnectionFromClient::prompt_save_file(i32 request_id, i32 window_server_client_id, i32 parent_window_id, String const& name, String const& ext, String const& path_to_view, Core::OpenMode const& requested_access)
{ {
auto relevant_permissions = requested_access & (Core::OpenMode::ReadOnly | Core::OpenMode::WriteOnly); auto relevant_permissions = requested_access & (Core::OpenMode::ReadOnly | Core::OpenMode::WriteOnly);
VERIFY(relevant_permissions != Core::OpenMode::NotOpen); VERIFY(relevant_permissions != Core::OpenMode::NotOpen);
@ -137,10 +137,10 @@ void ConnectionFromClient::prompt_save_file(i32 window_server_client_id, i32 par
auto user_picked_file = GUI::FilePicker::get_save_filepath(main_window, name, ext, path_to_view); auto user_picked_file = GUI::FilePicker::get_save_filepath(main_window, name, ext, path_to_view);
prompt_helper(user_picked_file, requested_access); prompt_helper(request_id, user_picked_file, requested_access);
} }
void ConnectionFromClient::prompt_helper(Optional<String> const& user_picked_file, Core::OpenMode const& requested_access) void ConnectionFromClient::prompt_helper(i32 request_id, Optional<String> const& user_picked_file, Core::OpenMode const& requested_access)
{ {
if (user_picked_file.has_value()) { if (user_picked_file.has_value()) {
VERIFY(user_picked_file->starts_with("/"sv)); VERIFY(user_picked_file->starts_with("/"sv));
@ -149,7 +149,7 @@ void ConnectionFromClient::prompt_helper(Optional<String> const& user_picked_fil
if (file.is_error()) { if (file.is_error()) {
dbgln("FileSystemAccessServer: Couldn't open {}, error {}", user_picked_file.value(), file.error()); dbgln("FileSystemAccessServer: Couldn't open {}, error {}", user_picked_file.value(), file.error());
async_handle_prompt_end(errno, Optional<IPC::File> {}, user_picked_file); async_handle_prompt_end(request_id, errno, Optional<IPC::File> {}, user_picked_file);
} else { } else {
auto maybe_permissions = m_approved_files.get(user_picked_file.value()); auto maybe_permissions = m_approved_files.get(user_picked_file.value());
auto new_permissions = requested_access & (Core::OpenMode::ReadOnly | Core::OpenMode::WriteOnly); auto new_permissions = requested_access & (Core::OpenMode::ReadOnly | Core::OpenMode::WriteOnly);
@ -158,10 +158,10 @@ void ConnectionFromClient::prompt_helper(Optional<String> const& user_picked_fil
m_approved_files.set(user_picked_file.value(), new_permissions); m_approved_files.set(user_picked_file.value(), new_permissions);
async_handle_prompt_end(0, IPC::File(file.value()->leak_fd(), IPC::File::CloseAfterSending), user_picked_file); async_handle_prompt_end(request_id, 0, IPC::File(file.value()->leak_fd(), IPC::File::CloseAfterSending), user_picked_file);
} }
} else { } else {
async_handle_prompt_end(-1, Optional<IPC::File> {}, Optional<String> {}); async_handle_prompt_end(request_id, -1, Optional<IPC::File> {}, Optional<String> {});
} }
} }

12
Userland/Services/FileSystemAccessServer/ConnectionFromClient.h
View file

@ -27,19 +27,19 @@ public:
private: private:
explicit ConnectionFromClient(NonnullOwnPtr<Core::Stream::LocalSocket>); explicit ConnectionFromClient(NonnullOwnPtr<Core::Stream::LocalSocket>);
virtual void request_file_read_only_approved(i32, i32, String const&) override; virtual void request_file_read_only_approved(i32, i32, i32, String const&) override;
virtual void request_file(i32, i32, String const&, Core::OpenMode const&) override; virtual void request_file(i32, i32, i32, String const&, Core::OpenMode const&) override;
virtual void prompt_open_file(i32, i32, String const&, String const&, Core::OpenMode const&) override; virtual void prompt_open_file(i32, i32, i32, String const&, String const&, Core::OpenMode const&) override;
virtual void prompt_save_file(i32, i32, String const&, String const&, String const&, Core::OpenMode const&) override; virtual void prompt_save_file(i32, i32, i32, String const&, String const&, String const&, Core::OpenMode const&) override;
void prompt_helper(Optional<String> const&, Core::OpenMode const&); void prompt_helper(i32, Optional<String> const&, Core::OpenMode const&);
RefPtr<GUI::Window> create_dummy_child_window(i32, i32); RefPtr<GUI::Window> create_dummy_child_window(i32, i32);
enum class ShouldPrompt { enum class ShouldPrompt {
No, No,
Yes Yes
}; };
void request_file_handler(i32, i32, String const&, Core::OpenMode const&, ShouldPrompt); void request_file_handler(i32, i32, i32, String const&, Core::OpenMode const&, ShouldPrompt);
virtual Messages::FileSystemAccessServer::ExposeWindowServerClientIdResponse expose_window_server_client_id() override; virtual Messages::FileSystemAccessServer::ExposeWindowServerClientIdResponse expose_window_server_client_id() override;

2
Userland/Services/FileSystemAccessServer/FileSystemAccessClient.ipc
View file

@ -1,4 +1,4 @@
endpoint FileSystemAccessClient endpoint FileSystemAccessClient
{ {
handle_prompt_end(i32 error, Optional<IPC::File> fd, Optional<String> chosen_file) =| handle_prompt_end(i32 request_id, i32 error, Optional<IPC::File> fd, Optional<String> chosen_file) =|
} }

8
Userland/Services/FileSystemAccessServer/FileSystemAccessServer.ipc
View file

@ -3,10 +3,10 @@
endpoint FileSystemAccessServer endpoint FileSystemAccessServer
{ {
request_file_read_only_approved(i32 window_server_client_id, i32 window_id, String path) =| request_file_read_only_approved(i32 request_id, i32 window_server_client_id, i32 window_id, String path) =|
request_file(i32 window_server_client_id, i32 window_id, String path, Core::OpenMode requested_access) =| request_file(i32 request_id, i32 window_server_client_id, i32 window_id, String path, Core::OpenMode requested_access) =|
prompt_open_file(i32 window_server_client_id, i32 window_id, String window_title, String path_to_view, Core::OpenMode requested_access) =| prompt_open_file(i32 request_id, i32 window_server_client_id, i32 window_id, String window_title, String path_to_view, Core::OpenMode requested_access) =|
prompt_save_file(i32 window_server_client_id, i32 window_id,String title, String ext, String path_to_view, Core::OpenMode requested_access) =| prompt_save_file(i32 request_id, i32 window_server_client_id, i32 window_id, String title, String ext, String path_to_view, Core::OpenMode requested_access) =|
expose_window_server_client_id() => (i32 client_id) expose_window_server_client_id() => (i32 client_id)
} }