Skip to content

Conversation

@snakefoot
Copy link
Contributor

See also #6022

@snakefoot snakefoot added the enhancement Improvement on existing feature label Oct 9, 2025
@coderabbitai
Copy link

coderabbitai bot commented Oct 9, 2025

Walkthrough

Updates MemoryTarget’s internal ThreadSafeList: modifies Add logic to purge excess items in a loop before appending, and restores a locking GetEnumerator that yields items safely during iteration.

Changes

Cohort / File(s) Summary
MemoryTarget ThreadSafeList updates
src/NLog/Targets/MemoryTarget.cs
- Add(T,int): replace single-item removal with while-loop removing items while count ≥ maxListCount, then add new item.
- Restore/GetEnumerator: reintroduce public enumerator that locks and yields items during iteration.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Caller
  participant ThreadSafeList as ThreadSafeList<T>
  participant InnerList as Inner List

  Caller->>ThreadSafeList: Add(item, maxListCount)
  activate ThreadSafeList
  ThreadSafeList->>ThreadSafeList: lock()
  loop while Count ≥ maxListCount
    ThreadSafeList->>InnerList: RemoveAt(0)
  end
  ThreadSafeList->>InnerList: Add(item)
  ThreadSafeList-->>ThreadSafeList: unlock()
  deactivate ThreadSafeList

  Caller->>ThreadSafeList: GetEnumerator()
  activate ThreadSafeList
  ThreadSafeList->>ThreadSafeList: lock()
  Note over ThreadSafeList: Yield items under lock
  ThreadSafeList-->>Caller: IEnumerator<T>
  ThreadSafeList-->>ThreadSafeList: unlock() on completion
  deactivate ThreadSafeList
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

I tidied lists with a rhythmic hop,
Old heads roll while new ones pop.
I guard the loop with gentle lock,
Each nibble counted, tick-tock, tick-tock.
Memory burrow neat and tight—
Add, enumerate, all done right.
Carrot commits approved tonight! 🥕🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description Check ❓ Inconclusive The description only contains a reference to another issue and does not summarize any of the changes in this PR, making it too vague to assess the intent or impact. Please expand the description to include a summary of the changes made to the ThreadSafeList Add method and the GetEnumerator logic to clarify the purpose of this PR.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title directly references MemoryTarget and the adjustment to MaxLogsCount behavior, which corresponds to the code changes that enforce log count limits on subsequent events. It is concise and specific enough for teammates to understand the primary change.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copy link

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a023270 and 6267be3.

📒 Files selected for processing (1)
  • src/NLog/Targets/MemoryTarget.cs (2 hunks)
🔇 Additional comments (1)
src/NLog/Targets/MemoryTarget.cs (1)

231-238: LGTM – Enumeration correctly holds lock.

The restored GetEnumerator() implementation correctly holds the lock during the entire enumeration (until the iterator completes or is disposed), which matches the documented behavior in the XML comments at lines 87-88 warning that enumeration blocks writes.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 9, 2025

@snakefoot snakefoot merged commit 5df46fa into NLog:dev Oct 10, 2025
5 of 6 checks passed
@snakefoot snakefoot added this to the 6.0.6 milestone Oct 24, 2025
@coderabbitai coderabbitai bot mentioned this pull request Nov 8, 2025
@snakefoot snakefoot changed the title MemoryTarget - Apply MaxLogsCount update on next LogEvent MemoryTarget - Apply MaxLogsCount limit on next LogEvent Nov 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Improvement on existing feature size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant