Repository navigation
refactor: remove all NOLINT suppressions, integrate EventBus, adopt std::source_location - #4
Merged
Merged
Conversation
The slot array and the payload of a shared memory object live in the segment, not inside the object, so they are addressed by a byte offset relative to the object (stable across mappings, unlike an absolute pointer). Restoring the address in the integer domain satisfied CodeQL and GCC but tripped clang-tidy's performance-no-int-to-ptr. Byte pointer arithmetic is not an option either: CodeQL reports cpp/suspicious-pointer-scaling, and GCC's -Wstringop-overflow then places the write inside the object and fails the build. std::bit_cast keeps the integer domain, produces no cast expression for clang-tidy and no pointer arithmetic for CodeQL. Drops four NOLINT suppressions.
The controller never evaluated the parameter - its groups are hardcoded in the member init list. The DI container resolves by type, so removing the parameter needs no wiring change. The test fixture loses its now unused GroupRepository member.
bugprone-exception-escape flags the implicitly generated move assignment: the shm::String members allocate from the segment, so a move cannot promise not to throw. Deleting the copy assignment is not an option (boost::container::vector::push_back requires it) and spelling out the special members would end aggregate initialisation. Declaring the destructor suppresses the implicit move operations instead; moves fall back to the copy operations, which is what the example does anyway.
Records the tool conflict behind the performance-no-int-to-ptr suppressions (clang-tidy vs. CodeQL vs. GCC), the remaining bugprone-easily-swappable-parameters sites, the HTTP/CLI integration gaps and the std::source_location migration surface.
The LOG_* macros forwarded __FILE__, __LINE__ and __PRETTY_FUNCTION__ to LoggerService::log(), and registerProperty() carried a file name plus a line number for the DataStorage identifier. Both now take a std::source_location. It is evaluated at the call site - inside the macros or as the default argument of registerProperty - so the reported file, line and function are unchanged, and with them the "<basename>:<line>" DataStorage identifier. LINTEL_FUNCTION is gone with the macros that used it; TypeName.h keeps __PRETTY_FUNCTION__ because a compile-time type name cannot come from a run-time object.
allOf took four identically typed strings whose order a caller can easily invert. The new aggregate names the four regular expressions once and carries the previous ".*" defaults, so short calls keep working. Follows the pattern from a4c1c59 (MessageQueueFilter, EventBusCapacities).
execute() registered a std::function through sqlite3_exec, passing it as a void pointer and tunnelling the rows through a callback whose two char ** parameters are indistinguishable. Replacing the callback is not an option: the signature belongs to the C API. The statement is now prepared and stepped directly, the way executeParameters() already works. sqlite3_exec runs every statement of a script, so the loop follows the pzTail out-parameter until the remainder is exhausted; a regression test covers a two-statement script. prepareStatement() takes the statement name as a string_view, which makes the two parameters distinguishable without touching any call site.
Four sites kept a NOLINT because two neighbouring parameters had the same type: - handleCommandInput()/handleTabInput() returned their prompt state through two bool & out-parameters; they now return an InputOutcome. - startsWith() takes the menu as a string_view - the two arguments are only compared. - parseAndScale() took the numeric part as a second string; the unit is now a named MemoryUnit table and the number is parsed with from_chars, which cannot throw and does not allocate. parseAndScale is stricter than before: from_chars rejects leading whitespace and a sign, where std::stoul accepted both. A memory size in the configuration is written without either, and the two new tests pin the rejection.
a4c1c59 replaced the byte offset with an absolute std::byte * to get rid of the int-to-pointer cast. That is only correct while the ring lives at the same address in every process, which a payload of a SharedMemoryRepository inside a segment does not: the segment is mapped at a different address per process, so the stored pointer belongs to the mapping of the process that constructed the ring. Back to the offset, this time materialised with std::bit_cast so that clang-tidy (performance-no-int-to-ptr), CodeQL (cpp/suspicious-pointer-scaling) and GCC (-Wstringop-overflow) are all satisfied - the same form EventBus::ShmRing uses. The new test copies a region holding the ring and its slots, wipes the original and checks that the ring still works; an absolute pointer fails it because it keeps pointing into the wiped region.
- grow_segment was described as "Shrink segment". - export_repository never bound --file even though it writes m_file; it now takes --file/-f. m_type was read nowhere and is gone. - PropertyCliComponent mixed --process-name and --process_name (and the same for the class name); underscores only belong in the XML and DB column names, every other CLI flag uses a dash. - UserManagementCliComponent declared AddGroup/RemoveGroup/UpdateGroup without a command or a handler. The HTTP API has no operation to create, delete or update a group, so the enum slots are removed rather than left as a promise the layer cannot keep. - HistoryCliComponent: the n_name/m_name typo and a stray ")" in the --service-name help text.
Both routes existed in MessageQueueController but were missing from swagger.yaml and its markdown mirror, so the wire contract advertised less than the server accepts. Status codes and payload fields follow the controller and the DTO serialisers; the collection endpoints stay documented in prose because a Swagger 2.0 response schema cannot express "array or paging envelope".
…queues
Three operations existed only in the service layer:
- SharedMemoryService::statsOf was declared on the concrete class only, so
no caller holding ISharedMemoryService could reach it. It is part of the
interface now, with the mock following.
- SharedMemoryCleanupService was not registered in the container at all.
It is, and the controller uses it for POST /shm/cleanup.
- MessageQueueService::recoverOrphans had no endpoint; POST
/messageQueue/recover returns the recovered count.
GET /shm/stats/{segment} keeps the /shm prefix of the neighbouring routes
rather than the /sharedMemory the plan suggested.
Each route has its Api method and a CLI command (show_stats, cleanup,
recover_message_queues).
The event bus had no HTTP, API or CLI surface at all - publishing, subscribing and inspecting a bus was only possible from C++. Seven routes now cover the observable part of IEventBus: list the configured buses, read one bus with its counters, publish, subscribe, unsubscribe, read a subscriber queue depth and reset. Admin-EventBus owns the mutating routes, User-EventBus may read and publish. EventBusService exposes what the controller needs (namesOf, configOf, busQueueDepthOf, subscriberQueueDepthOf) instead of reaching into its private maps, and the entry lookup moves into one helper. The CLI component mirrors the routes as show_event_buses, show_event_bus, publish_event, subscribe_event, unsubscribe_event, event_queue_depth and reset_event_bus. Subscriber and topic names are not listable - IEventBus only counts them.
CHANGELOG gains the added endpoints and CLI commands, the two breaking API changes (source_location, PropertyFilter), the stricter MemorySize parsing and the fixes; the plan document gets a status section that maps every bullet to its commit and separates what is deliberately open.
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Feature<T>::m_name was a std::string_view initialised from a temporary std::string, so it dangled as soon as the constructor returned. getName() handed out freed stack bytes, and StartupBuilder::verifyFeatureDependencies() - which compares the names at startup - aborted every process with "terminate called ... feature '<garbage>'". bin/main never started. Storing the name as a std::string is enough; getName() still returns a view into the member. The regression test scribbles over the stack between the constructor call and the assertion, so the dangling view is visible without a sanitizer.
export_repository replaced every "~" in the path with the home directory. On Windows the temporary directory is often a short name (C:\Users\RUNNER~1\...), so the path became C:\Users\RUNNERC:\Users\... and create_directories failed - the Windows job's only failure. expandHomeDirectory() replaces the tilde only when it starts the path and is followed by a separator (or ends it).
VirtualGroupBootstrapPlugin is a singleton inside the container and stored a shared_ptr to it, so container -> plugin -> container never dropped to zero. The container was therefore never destroyed, which means no service destructor ever ran - among them ProcessService's, which is where the worker processes were stopped. A shutdown of bin/main left both workers running as orphans. The plugin only needs the container during onStart(), so a weak_ptr with a lock() there is enough.
ProcessService only stopped its children in the destructor, so a child outlived its parent whenever the destructor did not run - which is exactly what the container reference cycle caused. stopAllProcesses() is now called from onShutdown() as well as from the destructor. It stops the monitor thread first (an autoRestart child that just exited would otherwise be spawned again between the stop and the terminate pass) and guards every stop/terminate call on its own, because stopOf() writes a history entry and the history service may already be down. The regression test starts a sleeper, calls onShutdown() and asserts the child is gone.
Same cycle as in VirtualGroupBootstrapPlugin: the worker registered a service that stored a shared_ptr to the container that owns it, so the worker's services were never destroyed either.
The status section still claimed the source_location changelog entry was open and did not know the three fixes that came out of running the examples (dangling feature name, tilde expansion, orphaned workers). The nine remaining items are now a backlog with priority, effort and prerequisites, and the decisions list points at it instead of repeating the already implemented A.2d question.
SharedMemoryRepositoryRegistry was dead code: nothing but its own test used it. SharedMemoryRepository<T>::of() now goes through it, keyed by "segment:name", so two call sites with the same name share one cell instead of building a second one (and getAs refuses a mismatched type instead of handing out a reinterpret_cast'ed pointer). The registry header moves to core/shm: the repository header is core and may not include a feature header, and the registry itself is stdlib-only.
AdminController had two routes and neither an API client nor a CLI command, so reload-tls and log-level were only reachable with curl. The client parses both answers instead of bailing out on every non-2xx status: reload and log-level answer 400/409 with a body that names the reason, and the CLI prints that reason. 401 still maps to the unauthorized exception, 403 and transport errors to a plain error line.
SharedMemoryService::recover() reclaims remap semaphores whose holder is
gone, but nothing ever called it - an operator had no way to trigger the
repeat that the doc comment promised. The route is admin only and answers
{"RECOVERED":n}; the CLI command is recover_segments.
The doc comments of recover() and of the opening path now say what the
code does: the lock is checked once when the segment is opened, and this is
the repeat for a segment that got stuck later.
Publishing rode on User-EventBus, which also grants read access - so every reader could inject events. The new virtual group Publisher-EventBus (or Admin-EventBus) is required for publish; User-EventBus stays read-only and gets 401 on publish, while a publisher may not read.
Two defects kept the shared memory IPC inside one process: ShmSegmentAccessor stored a raw allocator pointer, and a copy of it lives inside the segment. In a second process that pointer belongs to the first process' heap. The accessor now carries the segment *name* (40 bytes) and resolves it through a process-local binding, so a copy inside the segment works everywhere; the unnamed constructor keeps the old behaviour for process-local buffers and tests. MessageQueueShmCore kept its ring in a process-local registry, so every process built its own ring in the segment - the SHM backend was not an IPC mechanism at all. The header and the ring now live in the segment under a name, with a claim protocol for the first opener; the slot buffer stays offset-addressed relative to the ring. The local mutex/condition variable only affect wait latency (the wait already polls), not correctness. A segment written by an older build is recognised by a layout marker and recreated on open instead of being reinterpreted with the new layout.
- worker.cpp and VirtualGroupBootstrapPlugin took the container by value and moved it into a weak_ptr, which is a const reference - so the copy was pointless and the move did nothing. - ~ProcessService lost its try/catch when the stop loop moved into stopAllProcesses; a destructor must not throw. - the export test deleted the file while its ifstream was still open, which Windows refuses (POSIX does not care).
…ting The marker recreates a segment written by an older build; nothing is migrated. Say so in the log line, the changelog and the plan, and pin the behaviour with a test: a stale segment loses its named objects and gets the marker, a segment that already carries it is left alone.
Recreating a segment on a layout mismatch throws its contents away, and a segment may hold the only copy of something (the payloads of shm::SharedMemoryRepository<T>). The default is therefore a hard stop with the segment name and path, and the application picks one of three ways out: - shm::SegmentLayoutPolicy::setMigrator(): the hook runs before anything is removed, sees the untouched file plus the found and expected layout version, and decides by return value; it can export the payloads first. - SegmentLayoutPolicy::set(Recreate) or LINTEL_SHM_RECREATE=1: discard. - nothing: throw, leave the file alone for an offline migration. Four tests pin the matrix: refuse, recreate, migrator called with path and versions, and migrator refusing (file stays intact).
Documents which data in a segment is authoritative (the typed repository payloads are), what is derived (the property mirror) and what is volatile (event bus, queues), the three policies, how to write a migrator, and the path to per-object versioning.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Removes every
NOLINTin the tree, replaces__FILE__/__LINE__withstd::source_location, and closes the most valuable HTTP/CLI integration gaps. Plan and status:docs/nolint-integration-plan.md.87 files, 14 commits.
grep -rn NOLINT src/ examples/ tests/andgrep -rn __FILE__ src/ tests/ examples/ benchmarks/are both empty.What changed
A — NOLINT markers (13 → 0)
performance-no-int-to-ptrstd::bit_cast— the only form that satisfies all three tools at once: clang-tidy rejects the int→pointer cast, CodeQL reportscpp/suspicious-pointer-scalingfor byte-pointer arithmetic, and GCC's-Wstringop-overflowthen places the write inside the object and fails-Werrorbugprone-easily-swappable-parametersInputOutcomeinstead of twobool &, aMemoryUnittable +from_chars,std::string_view, aPropertyFilteraggregate, andsqlite3_execreplaced byprepare_v2/stepperformance-unnecessary-value-paramHistoryControllerdropped its unusedGroupRepositorybugprone-exception-escapeTestDataDtoexecute()keeps running multi-statement scripts: it follows thepzTailremainder ofsqlite3_prepare_v2, with a regression test for aCREATE+ 2×INSERTscript.B — integration
buses,bus/{name},publish,subscribePOST/DELETE,depth,reset) +EventBusApi+EventBusCliComponent(7 commands) + swagger + 35 tests.Admin-EventBusowns the mutating routes,User-EventBusmay read and publish.EventBusServicegainednamesOf,configOf,busQueueDepthOf,subscriberQueueDepthOfinstead of exposing its maps.GET /shm/stats/{segment},POST /shm/cleanup,POST /messageQueue/recover, each with its API method and CLI command.statsOfmoved intoISharedMemoryService,SharedMemoryCleanupServiceis registered in the container.grow_segmentsaid "Shrink segment",export_repositorynever bound--file(m_typewas dead),--process_namevs--process-name, threeUserManagementCliComponentenum slots without a command (the HTTP API cannot create groups — removed), ann_nametypo and a stray)in a help text, and/messageQueue/stats|purgewere missing from the spec.RingBufferShmbug:a4c1c59replaced the byte offset with an absolute pointer, which only works while the ring maps at the same address in every process — a repository payload inside a segment does not. Back to the offset withstd::bit_cast, plus a regression test that copies the region and reuses the ring at the second address.C —
std::source_location(51__FILE__→ 0)LOG_*/log()/logImpl(),registerProperty()(std::source_locationdefaulted tocurrent()) andDEFINE_PROPERTY. The 351LOG_*call sites are untouched, and the persistedDataStorageidentifier"<basename>:<line>"is byte-identical.LINTEL_FUNCTIONis gone;TypeName.hkeeps__PRETTY_FUNCTION__because a compile-time type name cannot come from a run-time object.Breaking changes
Listed under
[Unreleased]inCHANGELOG.md:PropertyRegistration::registerProperty()—(fileName, position)replaced bystd::source_location.PropertyRepository::allOf()— four strings replaced byPropertyFilter.Connection::prepareStatement()— statement name is astd::string_view.MemorySizerejects leading whitespace and a sign (from_charsinstead ofstoul).Verification
-Werror), 1247/1247 unit tests.check-format.sh master,check-namespace.sh,check-swagger.sh(47 paths),check-secrets.sh: green.Deliberately not in this PR
Documented in
docs/nolint-integration-plan.md§7:MessageQueueShmCoreis not cross-process (its registry is process-local, so each process builds its own ring),ShmSegmentAccessorstored inside a segment points at the creating process,SharedMemoryService::recover()still has no caller, and theProcessGrouplifecycle overloads, the MQ overflow/dead-letter policy andMessageQueueRequestReplyhave no endpoints yet. The first two block the segment-relative addressing that would replacestd::bit_castand need an own work package.