bugfix: Search results are mixed between tabs when doing Kad and ed2k searches - #530
Conversation
|
Nice catch on the dual-counter root cause. One thought on the chosen value: Same patch size with full structural safety: partition the 32-bit ID space by the top bit so the two allocators can never produce equal values, regardless of session length. -uint32_t CSearchManager::m_nextID = 0;
+// Top bit reserved for Kad-allocated IDs; ed2k Local/Global IDs
+// (CSearchDlg::StartNewSearch) live in the bottom half. Keeps the
+// two ID spaces from ever colliding regardless of session length.
+uint32_t CSearchManager::m_nextID = 0x80000000;Optionally also on the ed2k side at -m_nSearchID++;
+m_nSearchID = (m_nSearchID + 1) & 0x7fffffff;A couple of reasons this feels like the cleaner shape:
Happy to open this as a counter-PR off your branch if it's easier than amending here — your call. |
|
Posted #533 as an alternative — same bug, same dual-counter root cause you diagnosed (exactly right), but using a top-bit partition of the 32-bit ID space so the two counters can never alias regardless of session length. The +1024 offset here delays the collision; the partition prevents it structurally. Thanks for tracking this one down — your repro and analysis are what made the alternative possible. |
|
Hi got3nks, thanks for the review
I like your idea, just allow me a couple of hours, it can be further
improved. I will amend the PR.
|
a8e916b to
fa7f0d3
Compare
|
hi @got3nks I amended my commit and implemented your idea, which prevented the ed2k counter from ever entering kad counter space. As an extra modif, I removed the unused function SetNextSearchID(), as it could have a misleading behaviour now because of the mask. If someone ever needs it, they will arrive to the actual variable and mask with the explanatory comment. What are your thoughts? |
|
Yep, that's better — the OR-mask on each emit closes the (theoretical) wrap-around edge case ours has: once Removing the unused Going to close #533 in favor of yours; this is strictly a superset of what we proposed. |
|
Great work, guys! |
…ct#530) The Directories preferences panel was inert in amuleGUI: its tree browses the wrong filesystem when the core is remote, and nothing it produced reached the core (the shared-folder lists are not in the EC preferences packet, and SaveSharedFolders() is compiled out under CLIENT_GUI), while the reload sent alongside made the core rescan its own unchanged config — so it looked applied. Adds EC_OP_GET/SET_SHARED_DIRS carrying the explicit and recursive roots, negotiated via EC_TAG_CAN_SHAREDDIRS_CONFIG. The core validates each path, applies and persists the valid ones, rescans, and reports refused paths with a locale-neutral reason; the union file is refreshed alongside the intent lists so the rescan cannot trim the new roots back out. amuleGUI replaces the tree with a Path/Recursive list editor owned by CPreferencesRem, which outlives the dialog. The monolithic tree now tracks the roots it was painted from and rebuilds only when they move, and its pending-edit flag is cleared at session end instead of latching for the dialog lifetime. muuli_wdr.cpp moves from the muleappgui static library into GUI_SOURCES so each executable compiles it with its own defines; it was previously built once without CLIENT_GUI, making every such branch in it dead code. Closes amule-project#484.
…ect#533) The Flatpak job regenerates ECCodes.h from ECCodes.abstract rather than using the committed header, and amule-project#530's comments in that file made the generated header unparseable ("invalid preprocessing directive #=" plus a truncated ECOpCodes enum), failing every Flatpak build on master. The generator only treats a line as a comment when it matches "^#" with no leading whitespace, so the comments indented to line up with the tag entries fell through to the data path and had every word turned into a field. It also flattens the file into a CMake list by replacing newlines with ";", so a semicolon inside a comment splits the line and the remainder loses its leading "#". Documentation for these codes lives in the hand-maintained header, so the abstract keeps to entries. Generating before and after: master produces 8 mangled lines and 11 compile errors, this produces none, with all seven new codes intact.
…-project#531) Follow-up to amule-project#530, which made the core's share roots reachable over EC. amuleapi could list the files a share produced and ask the core to re-walk its roots, but had no way to see or change which roots those were. Adds /shared/directories: GET returns the configured roots as {path, recursive} (GUEST, matching GET /shared and GET /preferences); PUT replaces the whole set, mirroring the core operation rather than hiding a read-modify-write; POST adds a single root and DELETE removes one, since the scripted case is a single folder. POST is idempotent so "ensure this is shared" repeats safely; DELETE 404s on an unconfigured path so a typo is visible. The core validates each path — a REST client cannot stat the core filesystem — applying the ones that pass and returning the rest in `rejected` with a reason, so one bad path never discards the edit. Reasons arrive as codes and are rendered by the API, keeping the core locale out of responses. POST and DELETE hold a mutex across their read and write, since SendRecvSerialized locks per roundtrip and two concurrent adds would otherwise lose one. Covered by a new curl smoke that snapshots and restores the operator configuration, 30/30 passing live.
…ale daemons A headless amuled started without LANG / LC_* (systemd, Docker) runs under the POSIX/C locale, whose ASCII codeset cannot represent accented or other non-ASCII UTF-8 filesystem paths. wxConvFileName then fails to open them, so an accented shared directory silently becomes invisible to the file scan — reproduced on glibc: `LC_ALL=C` finds 0 files in an accented share where a UTF-8 locale finds them. This newly bites the remote shared-folder config (amule-project#530), whose whole point is configuring a headless core, but it affects all of amuled's non-ASCII file handling. aMuleInitLocale() now promotes LC_CTYPE to UTF-8 (C.UTF-8, falling back to en_US.UTF-8) when the resolved codeset is bare ASCII, and exports it so the promotion survives wx re-resolving the locale from the environment during app init — a plain setlocale() alone is undone there. The guard is the codeset itself, so a deliberate UTF-8 or latin1 locale is never overridden, and musl (already UTF-8 in its C locale) never triggers it. Windows is exempt (wide-char filesystem APIs). Verified on an ARM64 glibc VM: with the fix an accented shared folder is found under a bare environment, an explicit `LC_ALL=C`, and healthy UTF-8 locales alike, with no change for a real locale (it_IT.UTF-8).
…ale daemons A headless amuled started without LANG / LC_* (systemd, Docker) runs under the POSIX/C locale, whose ASCII codeset cannot represent accented or other non-ASCII UTF-8 filesystem paths. wxConvFileName then fails to open them, so an accented shared directory silently becomes invisible to the file scan — reproduced on glibc: `LC_ALL=C` finds 0 files in an accented share where a UTF-8 locale finds them. This newly bites the remote shared-folder config (amule-project#530), whose whole point is configuring a headless core, but it affects all of amuled's non-ASCII file handling. aMuleInitLocale() now promotes LC_CTYPE to UTF-8 (C.UTF-8, falling back to en_US.UTF-8) when the resolved codeset is bare ASCII, and exports it so the promotion survives wx re-resolving the locale from the environment during app init — a plain setlocale() alone is undone there. The guard is the codeset itself, so a deliberate UTF-8 or latin1 locale is never overridden, and musl (already UTF-8 in its C locale) never triggers it. Windows is exempt (wide-char filesystem APIs). Verified on an ARM64 glibc VM: with the fix an accented shared folder is found under a bare environment, an explicit `LC_ALL=C`, and healthy UTF-8 locales alike, with no change for a real locale (it_IT.UTF-8).
…ale daemons (amule-project#542) A headless amuled started without LANG / LC_* (systemd, Docker) runs under the POSIX/C locale, whose ASCII codeset cannot represent accented or other non-ASCII UTF-8 filesystem paths. wxConvFileName then fails to open them, so an accented shared directory silently becomes invisible to the file scan — reproduced on glibc: `LC_ALL=C` finds 0 files in an accented share where a UTF-8 locale finds them. This newly bites the remote shared-folder config (amule-project#530), whose whole point is configuring a headless core, but it affects all of amuled's non-ASCII file handling. aMuleInitLocale() now promotes LC_CTYPE to UTF-8 (C.UTF-8, falling back to en_US.UTF-8) when the resolved codeset is bare ASCII, and exports it so the promotion survives wx re-resolving the locale from the environment during app init — a plain setlocale() alone is undone there. The guard is the codeset itself, so a deliberate UTF-8 or latin1 locale is never overridden, and musl (already UTF-8 in its C locale) never triggers it. Windows is exempt (wide-char filesystem APIs). Verified on an ARM64 glibc VM: with the fix an accented shared folder is found under a bare environment, an explicit `LC_ALL=C`, and healthy UTF-8 locales alike, with no change for a real locale (it_IT.UTF-8).
Search results are mixed between tabs when doing Kad and ed2k searches.
Steps to reproduce:
3.a. The new tab 'freebsd' gets a copy of the results of 'ubuntu'. The search count is set to zero
3.b. The previous tab 'ubuntu' gets a mix of the original 'ubuntu' and the new 'freebsd' results
The rootcause seems to be the search ID. Actually, there are two variables used to define this value, both starting at 0.
The one for ed2k is defined in SearchDlg.cpp --> CSearchDlg::StartNewSearch() --> static uint32 m_nSearchID = 0;
The one for Kad is defined in SearchManager.cpp --> uint32_t CSearchManager::m_nextID = 0;
A given tab gets assigned a search ID to retrieve the results. Since these counters are managed separately for ed2k and kad, we can get a collision and the tabs will display wrong results...
The proposed fix is to initialize the Kad counter to a high integer, for example 1024.
The rationale for this is:
TL;DR Search results are mixed between tabs because Kad and ed2k searchIDs are both initialized at 0 and collide. Fix it by initializing Kad ID at a different value