Unified Search Architecture Redesign#428
Conversation
This commit introduces a comprehensive redesign of aMule's search subsystem and resolves several critical bugs. ## New Features ### Unified Search Architecture - Modular search framework with pluggable engines (Local, Global, Kad) - Clean separation between controllers, packet builders, and result handlers - Centralized UnifiedSearchManager for coordinating search operations - Search state management with proper timeout handling - Search result caching for improved performance ### New Components - SearchStateManager: Manages search state transitions - SearchTimeoutManager: Handles search timeouts and auto-retry - SimpleSearchCache: Caches search results for reuse - ParallelSearch: Enables parallel Kad searches - Protocol conversion layer for ED2K/Kad interoperability ## Bug Fixes ### Search Issues - Fix Kad search stuck at "Searching" state - Fix search tabs showing incorrect status - Fix "More" button errors due to missing searchType conversion - Fix corrupted FileID in search results from truncated packets - Fix race conditions causing search initialization failures - Fix search type being lost during timeout handling ### Encoding Issues - Fix double-encoded UTF-8 in network data - Fix corrupted text from partial UTF-8 conversion failures - Remove Unicode replacement characters (U+FFFD) from log output - Fix format specifier mismatches and invalid Unicode crashes - Fix format string crashes in SearchList and codec logging ### Security & Stability - Add comprehensive input validation for network packets - Fix critical UDP socket crash in Boost ASIO implementation - Fix critical security vulnerabilities and race conditions ## Testing - Add unit tests for unified search core abstractions - Add ModernLoggingTest for logging infrastructure ## Documentation - Add comprehensive search architecture documentation - Add build guide and implementation summaries
- Replace deadline_timer with steady_timer (deprecated in Boost 1.66+) - Update timer expiration from expires_from_now() to expires_after() - Replace boost::posix_time::seconds with std::chrono::seconds This fixes compilation errors with modern Boost versions that removed the deprecated deadline_timer and posix_time APIs.
Replace deprecated strand::wrap() with boost::asio::bind_executor() and null_buffers() with socket async_wait() to eliminate compiler warnings. 🤖 Generated with CodeMate
There was a problem hiding this comment.
Why not bumping min version to 3.7 and using FindICU?
| find_package(maxminddb QUIET) | ||
| if (NOT maxminddb_FOUND) | ||
| # Try alternative spelling | ||
| find_package(MaxMindDB QUIET) |
There was a problem hiding this comment.
Shouldn't be needed as upstream creates maxminddb-config.cmake that's the right spelling, if anyone (any Distro/Packagemanager) renames it, not our problem.
| if (ENABLE_IP2COUNTRY) | ||
| message(FATAL_ERROR "**************************************************") | ||
| message(FATAL_ERROR "libmaxminddb not found but ENABLE_IP2COUNTRY is enabled") | ||
| message(FATAL_ERROR "Please install: sudo apt install libmaxminddb-dev") |
There was a problem hiding this comment.
I doubt this command is portable, just "please install" should be enough
| message(FATAL_ERROR "libmaxminddb not found but ENABLE_IP2COUNTRY is enabled") | ||
| message(FATAL_ERROR "Please install: sudo apt install libmaxminddb-dev") | ||
| message(FATAL_ERROR "**************************************************") | ||
| else() |
There was a problem hiding this comment.
As this file isn't even included when ENABLE-IP2COUNTRY is not set, the else branch is never reaches, so the complete if can be omitted.
|
|
||
| if (ENABLE_IP2COUNTRY) | ||
| add_library (GeoIP::Shared UNKNOWN IMPORTED) | ||
| if (maxminddb_FOUND) |
There was a problem hiding this comment.
Both if's can be omitted here, as this isn't reached if one of them is false.
| option (ENABLE_IP2COUNTRY "compile with GeoIP IP2Country library" ON) | ||
| option (ENABLE_MMAP "enable using mapped memory if supported") | ||
| option (ENABLE_NLS "enable national language support" ON) | ||
| option (ENABLE_SEARCH_WINDOW_DEBUG "enable search window debug logging" ON) |
There was a problem hiding this comment.
Debugging shouldn't be activated by default
| endif() | ||
|
|
||
| find_package (UPNP CONFIG) | ||
| # Skip find_package(UPNP CONFIG) because the system CMake config has broken include paths |
There was a problem hiding this comment.
If anything is wrong with the UPNP config, report it upstream. I created it because I wanted to be able to use it here and not fiddle around with pkg-config.
| ## Requirements | ||
|
|
||
| ### Common Requirements | ||
| - CMake >= 3.10 |
There was a problem hiding this comment.
Does this align with main CMakeLists.txt?
|
|
||
| ### Common CMake Options | ||
| ```bash | ||
| cmake .. -DCMAKE_BUILD_TYPE=Release -DENABLE_AMULECMD=ON -DENABLE_AMULEGUI=ON -DENABLE_DAEMON=ON -DENABLE_WEBSERVER=ON -DENABLE_CAS=ON -DENABLE_WXCAS=ON -DENABLE_ALCC=ON -DENABLE_ALC=ON |
There was a problem hiding this comment.
Or just -DBUILD_EVERYTHING=On
There was a problem hiding this comment.
Most of these files never existed here. And who should update external reference. This whole file isn't needed.
There was a problem hiding this comment.
Also not needed file.
There was a problem hiding this comment.
Did you throw an AI in the docs and told her to show off what can be done?
We don't want to replicate docs of external projects.
There was a problem hiding this comment.
Same, and who the hell is Architecture team?
|
I stopped reading all that stuff. Please update your local git-ignore and do your testing with out-of-tree builds. |
|
@sergiomb2 @RealGreenDragon @comio @stefanop @matoro Could you take a look at the code. It's way too much for just me. |
This is obvious AI junk. Why even bother? |
|
Because maxminddb is on my list for quite a while, and the search-improvements (parallel) could also be interesting. But I guess pulling this out will cause too much work also. |
|
@3togo Can you split that stuff into components that relate to each other and PR them separately?
And forget about converting stuff to cpp-20 just for converting reasons with no benefit like the modern-logging. And insert yourself into the copyright notice, you created them for "- 2011" in the name of aMule Team, angel vidal and Merkur. |
|
github says this PR has conflicts:
and please, try to avoid dirty commits like: Merge branch 'master' into master. I mean, you can merge master into master without merge commit. |
|
Hi, first , don't merge master , do git rebase upstream/master instead, the last two commit could be a separated PR
I'd like to test them |
|
Hi, Please don’t merge master into your branch. Instead, rebase onto upstream/master: Also, the last two commits should be submitted as a separate PR, since they are logically independent from the rest of the changes. And thanks to my friend ChatGPT for the translation and snippets. What is written above is therefore not entirely my responsibility, so use it with appropriate caution. |
|
@3togo Two of the ideas in this PR have been split out and reworked as smaller standalone PRs against current master:
Both kept the core ideas but shed the abstractions, the unified-search architecture, and the assorted committed build artifacts so each diff stays narrow enough for one-pass review. |
mrjimenez
left a comment
mrjimenez
left a comment
There was a problem hiding this comment.
Thank you for the contribution. The ideas behind this PR — parallel Kad search, MaxMindDB backend, Boost.Asio modernisation — are genuinely interesting. However the submission has several blocking issues that must be resolved before code review is possible.
Blocking issues
1. Build artifacts committed
CMake-generated files (docs/CMakeFiles/, po/CMakeFiles/, src/libs/CMakeFiles/, cmake_install.cmake, progress.marks, DependInfo.cmake, etc.) and compiled binaries (final_test, simple_test, simulate_crash, performance_benchmark) and test output (localglobal_good.txt) were committed alongside the source. These must be removed from git history. Updating .gitignore alone is not enough once files are already tracked.
Fix:
git rm -r --cached docs/CMakeFiles/ po/CMakeFiles/ src/libs/CMakeFiles/ \
final_test simple_test simulate_crash performance_benchmark localglobal_good.txtBuild out-of-tree (cmake -B build/ from the repo root) so the working tree stays clean.
2. Developer scripts do not belong in the repo
resolve_conflicts.sh, run_amule_clean.sh, run_amule_silent.sh are personal workflow scripts. Please remove them.
3. Branch has merge conflicts and a dirty merge commit
As @sc0w noted, the branch cannot be rebased due to conflicts. The history also contains a "Merge branch 'master' into master" commit (2cc6ffc). Please rebase:
git rebase upstream/master
git push --force4. PR scope is not reviewable (310 files, +37 K lines)
The GitHub diff API returns HTTP 406 — the diff literally cannot be rendered. As already requested by @Vollstrecker and @sergiomb2, please split this into focused PRs. Note that @got3nks has already extracted the two most-requested pieces:
Additional issues to address before re-submission
5. Missing author copyright. All new files carry the 2003-2011 aMule Team / 2002-2011 Merkur headers verbatim. Please add your own copyright line with the correct year (as @Vollstrecker already requested).
6. Naming convention. The codebase uses PascalCase for all method names. The new src/search/ architecture uses camelCase throughout (startSearch(), stopSearch(), getSearchId(), …). New code should match the existing convention.
7. New namespace search is not used anywhere else in the codebase. If namespaces are to be introduced, that is a separate, codebase-wide decision.
8. Duplicate class at two paths. Both src/search/UnifiedSearchManager and src/search/unified/manager/UnifiedSearchManager exist. Only one should.
Once the above are addressed, we can review follow-on PRs.
7 participants
Summary
This PR introduces a comprehensive redesign of aMule's search subsystem, implementing a unified search architecture that improves reliability, maintainability, and user experience. The changes address several long-standing issues with search functionality, including stuck searches, race conditions, and inconsistent state management.
Key Changes
1. Unified Search Architecture
New modular search framework with pluggable search engines (Local, Global, Kad)
Clean separation between search controllers, packet builders, and result handlers
Centralized UnifiedSearchManager for coordinating all search operations
2. New Components
SearchStateManager - Manages search state transitions
SearchTimeoutManager - Handles search timeouts and auto-retry
SimpleSearchCache - Caches search results for reuse
ParallelSearch - Enables parallel Kad searches