-
Notifications
You must be signed in to change notification settings - Fork 7
Fix hydration race, socket framing, and cross-desktop ignore defaults #54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
toxicphreAK
wants to merge
6
commits into
opencloud-eu:main
from
toxicphreAK:fix/integration-findings
Closed
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
71ebc14
Fix inverted pin state names reported by openvfs_stat
toxicphreAK 0de6fdc
Reassemble socket API messages across reads
toxicphreAK 4305cf3
Bound the hydration wait and close the open() registration race
toxicphreAK 986cd0e
Ship a cross-desktop ignoreApps default
toxicphreAK 1c270b2
Make SPDX headers consistent and complete
toxicphreAK 0096111
Add a test for the socket framing and the hydration wait
toxicphreAK File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| # The socket and job handling live in the openvfsfuse executable, so the test | ||
| # compiles those sources directly rather than linking a library. | ||
| add_executable(socketthreadtest | ||
| socketthreadtest.cpp | ||
| ../socketthread.cpp | ||
| ../sharedmap.cpp | ||
| ../strtools.cpp | ||
| ) | ||
| target_include_directories(socketthreadtest PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/..) | ||
| target_link_libraries(socketthreadtest PRIVATE nlohmann_json::nlohmann_json Threads::Threads) | ||
| ecm_mark_as_test(socketthreadtest) | ||
|
|
||
| add_test(NAME socketthreadtest COMMAND socketthreadtest) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,169 @@ | ||
| // SPDX-License-Identifier: GPL-3.0-or-later | ||
| // SPDX-FileCopyrightText: 2026 Klaas Freitag <k.freitag@opencloud.eu> | ||
|
|
||
| /* | ||
| * Drives SocketThread and SharedMap over a real AF_UNIX socket, standing in for | ||
| * the desktop client. Covers the stream framing and the hydration wait, both of | ||
| * which are timing dependent and regress silently. | ||
| */ | ||
|
|
||
| #include "sharedmap.h" | ||
| #include "socketthread.h" | ||
|
|
||
| #include <chrono> | ||
| #include <cstring> | ||
| #include <filesystem> | ||
| #include <iostream> | ||
| #include <memory> | ||
| #include <string> | ||
| #include <sys/socket.h> | ||
| #include <sys/un.h> | ||
| #include <thread> | ||
| #include <unistd.h> | ||
|
|
||
| using namespace std::chrono_literals; | ||
|
|
||
| namespace { | ||
|
|
||
| int failures = 0; | ||
|
|
||
| void check(bool ok, const std::string &what) | ||
| { | ||
| std::cerr << (ok ? "ok : " : "FAIL : ") << what << std::endl; | ||
| if (!ok) { | ||
| ++failures; | ||
| } | ||
| } | ||
|
|
||
| /// A socket path has to fit into sockaddr_un::sun_path, so keep it short and | ||
| /// out of the (potentially deeply nested) build directory. | ||
| std::string makeSocketPath() | ||
| { | ||
| char tmpl[] = "/tmp/openvfs-test-XXXXXX"; | ||
| const char *dir = mkdtemp(tmpl); | ||
| if (!dir) { | ||
| std::cerr << "Failed to create a temporary directory" << std::endl; | ||
| std::exit(2); | ||
| } | ||
| return std::string(dir) + "/s"; | ||
| } | ||
|
|
||
| int listenOn(const std::string &path) | ||
| { | ||
| const int fd = socket(AF_UNIX, SOCK_STREAM, 0); | ||
| sockaddr_un addr{}; | ||
| addr.sun_family = AF_UNIX; | ||
| std::strncpy(addr.sun_path, path.c_str(), sizeof(addr.sun_path) - 1); | ||
| if (bind(fd, reinterpret_cast<sockaddr *>(&addr), sizeof(addr)) != 0 || listen(fd, 1) != 0) { | ||
| std::cerr << "Failed to listen on " << path << ": " << std::strerror(errno) << std::endl; | ||
| std::exit(2); | ||
| } | ||
| return fd; | ||
| } | ||
|
|
||
| void writeAll(int fd, const std::string &data) | ||
| { | ||
| size_t offset = 0; | ||
| while (offset < data.size()) { | ||
| const ssize_t n = write(fd, data.data() + offset, data.size() - offset); | ||
| if (n <= 0) { | ||
| return; | ||
| } | ||
| offset += static_cast<size_t>(n); | ||
| } | ||
| } | ||
|
|
||
| std::string hydrateResult(int id, const std::string &argumentsBody) | ||
| { | ||
| return "V2/HYDRATE_FILE_RESULT:{\"id\":\"" + std::to_string(id) + "\",\"arguments\":{" + argumentsBody + "}}\n"; | ||
| } | ||
|
|
||
| } | ||
|
|
||
| int main() | ||
| { | ||
| const std::string socketPath = makeSocketPath(); | ||
| const int server = listenOn(socketPath); | ||
|
|
||
| SharedMap jobs; | ||
| SocketThread socketThread("TestSocketThread", jobs); | ||
| socketThread.CreateThread(socketPath); | ||
|
|
||
| const int client = accept(server, nullptr, nullptr); | ||
| if (client < 0) { | ||
| std::cerr << "SocketThread did not connect" << std::endl; | ||
| return 2; | ||
| } | ||
|
|
||
| // Register a job the way openVFSfuse_open() does, before posting it. | ||
| const auto post = [&](int id) { | ||
| auto data = std::make_shared<MsgData>(); | ||
| data->msg = "V2/HYDRATE_FILE"; | ||
| data->file = "/tmp/some/file"; | ||
| data->id = id; | ||
| jobs.insert(id, HydJob{.state = HydJobState::Running}); | ||
| socketThread.PostMsg(data); | ||
| }; | ||
|
|
||
| // A message split across two writes must not be acted on until it is complete. | ||
| writeAll(client, "VERSION:1.2.3:2"); | ||
| std::this_thread::sleep_for(750ms); | ||
| check(jobs.desktopClientPid() == 0, "an incomplete message is not dispatched"); | ||
| writeAll(client, ":4242\n"); | ||
| std::this_thread::sleep_for(750ms); | ||
| check(jobs.desktopClientPid() == 4242, "a message split across two writes is reassembled"); | ||
|
|
||
| // A reply well past any single read buffer must arrive as one message. An | ||
| // error string carrying a path and a description clears 1 KB easily. | ||
| post(101); | ||
| writeAll(client, hydrateResult(101, "\"error\":\"" + std::string(8000, 'E') + "\"")); | ||
| check(jobs.waitForJob(101, 10s) == HydJobResult::Failed, "an 8 KB reply is parsed as a single message"); | ||
|
|
||
| // Several replies batched into one write must all be dispatched. | ||
| post(102); | ||
| post(103); | ||
| writeAll(client, hydrateResult(102, "\"status\":\"OK\"") + hydrateResult(103, "\"status\":\"FAILED\"")); | ||
| check(jobs.waitForJob(102, 10s) == HydJobResult::Succeeded, "first of two batched replies is dispatched"); | ||
| check(jobs.waitForJob(103, 10s) == HydJobResult::Failed, "second of two batched replies is dispatched"); | ||
|
|
||
| // ... and the stream is still in sync after all of the above. | ||
| post(104); | ||
| writeAll(client, hydrateResult(104, "\"status\":\"OK\"")); | ||
| check(jobs.waitForJob(104, 10s) == HydJobResult::Succeeded, "the stream stays in sync"); | ||
|
|
||
| // A silent client must time out, bounded by wall-clock time. | ||
| post(105); | ||
| const auto beforeTimeout = std::chrono::steady_clock::now(); | ||
| const auto timedOut = jobs.waitForJob(105, 500ms); | ||
| const auto waited = std::chrono::steady_clock::now() - beforeTimeout; | ||
| check(timedOut == HydJobResult::TimedOut, "a silent client is reported as a timeout"); | ||
| check(waited >= 450ms && waited < 30s, "the timeout is bounded by wall-clock time"); | ||
|
|
||
| // A reply arriving later must be observed without waiting out a long backoff. | ||
| post(106); | ||
| std::thread late([&] { | ||
| std::this_thread::sleep_for(300ms); | ||
| writeAll(client, hydrateResult(106, "\"status\":\"OK\"")); | ||
| }); | ||
| const auto beforeLate = std::chrono::steady_clock::now(); | ||
| const auto lateResult = jobs.waitForJob(106, 60s); | ||
| const auto latency = std::chrono::steady_clock::now() - beforeLate; | ||
| late.join(); | ||
| check(lateResult == HydJobResult::Succeeded, "a late reply is picked up"); | ||
| check(latency < 10s, "a late reply is not delayed by a growing backoff"); | ||
|
|
||
| // A caller must never be left waiting for a message that was never queued. | ||
| socketThread.ExitThread(); | ||
| auto dropped = std::make_shared<MsgData>(); | ||
| dropped->msg = "V2/HYDRATE_FILE"; | ||
| dropped->id = 107; | ||
| check(!socketThread.PostMsg(dropped), "PostMsg reports messages dropped during shutdown"); | ||
|
|
||
| close(client); | ||
| close(server); | ||
| std::error_code ec; | ||
| std::filesystem::remove_all(std::filesystem::path(socketPath).parent_path(), ec); | ||
|
|
||
| std::cerr << (failures == 0 ? "All checks passed" : std::to_string(failures) + " check(s) failed") << std::endl; | ||
| return failures == 0 ? 0 : 1; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,38 @@ | ||
| { | ||
| "ignoreApps": { | ||
| "byName": [ | ||
| "/usr/bin/dolphin" | ||
| ], | ||
| "endsWith": [ | ||
| "dolphin", | ||
| "kioworker", | ||
| "baloo_file" | ||
| "baloo_file", | ||
| "baloo_file_extractor", | ||
| "nautilus", | ||
| "nemo", | ||
| "caja", | ||
| "thunar", | ||
| "pcmanfm", | ||
| "pcmanfm-qt", | ||
| "thumbnailer", | ||
| "gnome-thumbnail-font", | ||
| "gnome-thumbnail-factory", | ||
| "tumblerd", | ||
| "tracker-miner-fs", | ||
| "tracker-miner-fs-3", | ||
| "tracker-extract", | ||
| "tracker-extract-3", | ||
| "localsearch-3", | ||
| "localsearch-extractor-3", | ||
| "gvfsd", | ||
| "gvfsd-metadata", | ||
| "gvfsd-trash", | ||
| "updatedb", | ||
| "updatedb.mlocate", | ||
| "updatedb.plocate", | ||
| "clamd", | ||
| "clamscan", | ||
| "freshclam" | ||
| ] | ||
| } | ||
| }, | ||
| "hydrationTimeoutSeconds": 300 | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change, without many benefits.