Skip to content

src: replace duplicate loop hook regs#444

Merged
santigimeno merged 1 commit intonode-v24.x-nsolid-v6.xfrom
santi/dynamic_blocked_loop_threshold
Mar 31, 2026
Merged

src: replace duplicate loop hook regs#444
santigimeno merged 1 commit intonode-v24.x-nsolid-v6.xfrom
santi/dynamic_blocked_loop_threshold

Conversation

@santigimeno
Copy link
Copy Markdown
Member

@santigimeno santigimeno commented Mar 23, 2026

Replace blocked and unblocked loop hook entries when the same callback is registered again.

These hooks were stored in append-only TSList containers, so dynamic reconfiguration in agents like gRPC and ZMQ kept stale registrations alive. That caused duplicate callback delivery and let old blocked loop thresholds continue affecting detection.

Add TSList::replace_if() and use it in blocked and unblocked loop hook registration. When the callback function pointer matches an existing entry, overwrite it instead of appending a new one.

For blocked loop hooks, recompute min_blocked_threshold_ after each registration so a replaced threshold immediately becomes effective.

Summary by CodeRabbit

  • Bug Fixes

    • Prevented duplicate blocked-loop hook entries when re-registering the same hook; ensures minimum blocked threshold is recalculated correctly across active hooks.
  • Tests

    • Expanded blocked-loop tests to validate multiple unblock cycles and varied threshold configurations for both main thread and worker scenarios.

@santigimeno santigimeno requested a review from RafaelGSS March 23, 2026 09:38
@santigimeno santigimeno self-assigned this Mar 23, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 23, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0fc86146-6f8f-4dc6-ae9c-04c384b2375d

📥 Commits

Reviewing files that changed from the base of the PR and between cc47380 and b99a8a2.

📒 Files selected for processing (4)
  • src/nsolid/nsolid_api.cc
  • src/nsolid/nsolid_api.h
  • src/nsolid/thread_safe.h
  • test/agents/test-grpc-blocked-loop.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/nsolid/nsolid_api.h
  • test/agents/test-grpc-blocked-loop.mjs

Walkthrough

Refactors blocked-loop hook management to replace existing hooks with matching callbacks instead of duplicating, adds a helper to recompute the minimum blocked threshold, provides a thread-safe replace_if() utility, and updates tests to exercise multi-cycle blocked/unblocked hook emissions with different thresholds.

Changes

Cohort / File(s) Summary
Thread-safe list utility
src/nsolid/thread_safe.h
Added TSList<DataType>::replace_if(Match match, DataType&& data) which replaces the first matching element under lock or appends if none match.
Hook management refactor
src/nsolid/nsolid_api.cc, src/nsolid/nsolid_api.h
EnvList::OnBlockedLoopHook and OnUnblockedLoopHook now use replace_if() to avoid duplicate entries; introduced EnvList::refresh_min_blocked_threshold() to recompute min_blocked_threshold_ by scanning stored blocked hooks.
Blocked-loop tests expanded
test/agents/test-grpc-blocked-loop.mjs
Reworked main-thread and worker tests into two-cycle flows to assert multiple loop_unblocked emissions and added cases with custom NSOLID_BLOCKED_LOOP_THRESHOLD env values.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I hopped through lists and swapped a pair,
Replaced the duplicates with careful care.
Thresholds counted, refreshed anew,
Tests now wait for unblocks times two.
🥕✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 9.09% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'src: replace duplicate loop hook regs' accurately summarizes the main change: replacing duplicate loop hook registrations with overwrite logic via TSList::replace_if().

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch santi/dynamic_blocked_loop_threshold

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
test/agents/test-grpc-blocked-loop.mjs (1)

158-158: Minor: Add semicolon for consistency.

The debug log is fine for test visibility, but the missing semicolon is inconsistent with the rest of the file.

Suggested fix
-          console.log(times)
+          console.log(times);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/agents/test-grpc-blocked-loop.mjs` at line 158, Add the missing
semicolon to the debug statement: update the console.log(times) call in
test-grpc-blocked-loop.mjs (the console.log invocation used for test visibility)
to end with a semicolon to match the file's style and maintain consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@test/agents/test-grpc-blocked-loop.mjs`:
- Line 158: Add the missing semicolon to the debug statement: update the
console.log(times) call in test-grpc-blocked-loop.mjs (the console.log
invocation used for test visibility) to end with a semicolon to match the file's
style and maintain consistency.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 79c04b91-0e47-4b5f-99d9-07e3df3a9261

📥 Commits

Reviewing files that changed from the base of the PR and between d41bff4 and cc47380.

📒 Files selected for processing (4)
  • src/nsolid/nsolid_api.cc
  • src/nsolid/nsolid_api.h
  • src/nsolid/thread_safe.h
  • test/agents/test-grpc-blocked-loop.mjs

coderabbitai[bot]
coderabbitai bot previously approved these changes Mar 23, 2026
@santigimeno santigimeno requested a review from EHortua March 23, 2026 14:34
Copy link
Copy Markdown

@EHortua EHortua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working fine

Replace blocked and unblocked loop hook entries when the same callback
is registered again.

These hooks were stored in append-only TSList containers, so dynamic
reconfiguration in agents like gRPC and ZMQ kept stale registrations
alive. That caused duplicate callback delivery and let old blocked loop
thresholds continue affecting detection.

Add TSList::replace_if() and use it in blocked and unblocked loop
hook registration. When the callback function pointer matches an
existing entry, overwrite it instead of appending a new one.

For blocked loop hooks, recompute min_blocked_threshold_ after each
registration so a replaced threshold immediately becomes effective.

PR-URL: #444
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: EHortua <55801532+EHortua@users.noreply.github.com>
@santigimeno santigimeno force-pushed the santi/dynamic_blocked_loop_threshold branch from b99a8a2 to 20e6cad Compare March 31, 2026 09:50
@santigimeno santigimeno merged commit 20e6cad into node-v24.x-nsolid-v6.x Mar 31, 2026
13 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants