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. |
5 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