fix: give the config lock a context and require it to tolerate blocking - #758
Conversation
Pool cleanup runs inside SolidSyslog_LockConfig, and adapter cleanup can block — closing a transport or a file. The documented primitive was a critical section, which forbids blocking, so a shipped adapter's Destroy could hang the target. The lock is now specified as a host mutex that may be held while blocking, and never a SolidSyslog Mutex, whose Create walks a pool under this lock. SolidSyslogConfigLockFunction takes a void* context and SolidSyslog_SetConfigLock installs one alongside the pair, so the handle no longer needs a file-scope static in integrator code. SolidSyslog_LockConfig and SolidSyslog_UnlockConfig keep their signatures, so no pool class changes.
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe configuration lock API now accepts a caller-provided context pointer and passes it to lock and unlock callbacks. Tests cover context forwarding and null-handler restoration. Documentation requires a blocking-capable host lock for cleanup under the configuration lock. ChangesConfiguration lock context support
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Around line 730-731: Update the documentation sentence around
SolidSyslogConfigLock.h to explicitly identify the installed lock as the
subject: the installed lock must tolerate blocking and must not be a SolidSyslog
Mutex. Avoid using an ambiguous “it” that could refer to cleanup.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 42800cf3-cc44-4aa0-a4a5-460a13c61692
📒 Files selected for processing (6)
CLAUDE.mdCore/Interface/SolidSyslogConfigLock.hCore/Source/SolidSyslogConfigLock.cTests/SolidSyslogConfigLockTest.cppTests/Support/ConfigLockFake.cdocs/porting.md
|
@coderabbitai pause |
✅ Action performedReviews paused. |
☀️ Quality Summary Created by Quality Monitor v4.15.0 (#82d77af). More details are shown in the GitHub Checks Result. |
Purpose
Closes #754.
SolidSyslogPoolAllocator_FreeIfInUseruns the adapter's cleanup callbackbetween
SolidSyslog_LockConfig()andSolidSyslog_UnlockConfig(), and adaptercleanup can block —
Closeon a transport or a file. The primitive the librarydocumented was a critical section, which forbids blocking, so a shipped
adapter's
Destroycould hang the target.Change Description
The call chain is unchanged: cleanup still runs inside the lock. What changes is
what the lock is specified to be, so the issue closes on the contract rather
than on the chain.
SolidSyslogConfigLock.hnow requires a host mutex that may be held whileblocking, and prohibits a SolidSyslog Mutex, whose
Createwalks a pool underthis same lock.
SolidSyslogConfigLockFunctiontakes avoid* contextandSolidSyslog_SetConfigLockinstalls one alongside the pair, so an integratorwiring a mutex handle needs no file-scope static. That follows the callback
convention in CLAUDE.md, which migrates a context-less callback when its owner
is next modified.
SolidSyslog_LockConfigandSolidSyslog_UnlockConfigkeep their signatures,so none of the pool classes change.
The header is also now free of platform references, per the Core sweep in
progress.
docs/porting.mdand CLAUDE.md drop their own copies of therecommendation and point at the header. The Cleanup runs under the config lock
invariant added by #757 is deleted: it named a FreeRTOS critical section, cited
this issue as open, and its content is now covered by the slot-walk section
above it and by Bounded blocking below it.
Breaking:
SolidSyslog_SetConfigLockgains a parameter, and the callbacktypedef gains one. Integrator wiring fails to compile until updated, which is
the right way for it to break. Pre-1.0, and on the 0.1.0 milestone.
Test Evidence
Red-green on the lock side: a test asserting the installed context reaches the
lock callback failed to compile against the two-parameter signature, then passed
once the typedef, the installer and the stored context were in place. The
matching unlock test passed on arrival — the shared typedef had already forced
SolidSyslog_UnlockConfigto pass the context — so it is a characterisationtest, not a cycle.
FreeIfInUseInvokesCleanupWhileHoldingTheLockinSolidSyslogPoolAllocatorTest.cppalready pins the invariant the new contractrests on, and still passes. No unit test can prove a blocking cleanup completes
under a real primitive; that needs a scheduler, and is not covered here.
debugpreset green (both executables), markdownlint clean on the two changed.mdfiles,scripts/check_platform_docs.pypasses.clangandfreertos-hostnot run locally — left to CI.Areas Affected
Core/Interface/SolidSyslogConfigLock.handCore/Source/SolidSyslogConfigLock.c(tier 1), the config-lock test and its fake,
docs/porting.md, CLAUDE.md.Integrators who install a config lock must update the call and review the
primitive they installed.
Summary by CodeRabbit