NullTarget - Improve thread concurrency when FormatMessage = false by snakefoot · Pull Request #6036 · NLog/NLog

snakefoot · 2025-11-01T11:21:59Z

No description provided.

coderabbitai · 2025-11-01T11:22:09Z

Walkthrough

Two protected override async write handlers were added to NullTarget: they check the FormatMessage flag and either delegate to the base implementation or increment a thread-safe counter and complete continuations without formatting.

Changes

Cohort / File(s)	Summary
NullTarget async write handlers `src/NLog/Targets/NullTarget.cs`	Added `protected override void WriteAsyncThreadSafe(AsyncLogEventInfo logEvent)` and `protected override void WriteAsyncThreadSafe(IList<AsyncLogEventInfo> logEvents)`. Each checks `FormatMessage`: if true, delegates to base; if false, increments a thread-safe `_logEventCounter` and completes continuations. Added `using System.Collections.Generic`, `using System.Threading`, and `using NLog.Common`.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    actor Logger
    participant NullTarget
    participant BaseTarget as Base
    note right of NullTarget #DDFFDD: New WriteAsyncThreadSafe overrides

    Logger->>NullTarget: WriteAsyncThreadSafe(event or events)
    alt FormatMessage == true
        NullTarget->>Base: base.WriteAsyncThreadSafe(...)
        Base-->>NullTarget: Completed (may format)
        NullTarget-->>Logger: Continuation completed
    else FormatMessage == false
        NullTarget->>NullTarget: Interlocked.Add/_logEventCounter++\nComplete continuations with null
        NullTarget-->>Logger: Continuation completed
    end

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Single file change with two similarly patterned overrides.
Things to check:
- Correct use of Interlocked for thread-safe counter updates.
- Proper completion of associated AsyncLogEventInfo.Continuation (no exception swallowing).
- Added using directives don't conflict with existing names.

Poem

🐰 I nibble bytes and tally hops,
Two handlers added — no message stops.
If formatting sleeps, counters leap,
Continuations wake from gentle sleep.
A small null hop, a quiet keep.

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description Check	❓ Inconclusive	The pull request has no description provided by the author. While the check instructions note this should be "very lenient" and pass as long as it's not off-topic, an empty description conveys no meaningful information about the changeset and technically does not describe any part of the implementation. This falls into the ambiguous territory where the description is extremely vague (non-existent) and while not off-topic, it provides no information to verify relatedness to the changes.	The author should consider adding a pull request description that explains the motivation, benefits, and implementation details of the thread concurrency improvements to NullTarget. This would help reviewers understand the context and rationale for the changes, even though the title itself is clear and informative.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The pull request title "NullTarget - Improve thread concurrency when FormatMessage = false" directly and accurately summarizes the main changes in the changeset. The modifications add two protected override async write handlers to NullTarget that specifically check the FormatMessage flag and implement thread-safe counting operations when it is false. The title is specific, clear, and concise without being vague or misleading, effectively communicating the primary purpose of the changes to anyone reviewing the repository history.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1f94a3c and e3a9dbd.

📒 Files selected for processing (1)

src/NLog/Targets/NullTarget.cs (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/NLog/Targets/NullTarget.cs (2)

src/NLog/Targets/AsyncTaskTarget.cs (2)

WriteAsyncThreadSafe (352-367)

WriteAsyncThreadSafe (372-375)

src/NLog/Targets/Target.cs (2)

WriteAsyncThreadSafe (610-623)

WriteAsyncThreadSafe (648-664)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: task-list-completed

🔇 Additional comments (3)

src/NLog/Targets/NullTarget.cs (3)

36-39: LGTM! Required using directives for the new async methods.

The added using directives are necessary to support the new WriteAsyncThreadSafe overrides: System.Collections.Generic for IList, System.Threading for Interlocked operations, and NLog.Common for AsyncLogEventInfo.

108-120: Excellent! Critical issue resolved - continuation is now properly invoked.

The implementation correctly addresses the critical issue from the previous review. When FormatMessage is false, the method now:

Atomically increments the counter using Interlocked.Increment (thread-safe)

Invokes logEvent.Continuation(null) to signal completion

This optimization bypasses the base implementation's lock when message formatting is disabled, achieving the PR's performance objective while maintaining correctness.

122-137: Excellent! Critical issue resolved - all continuations are now properly invoked.

The batch implementation correctly addresses the critical issue from the previous review. When FormatMessage is false, the method now:

Atomically adds the batch count using Interlocked.Add (thread-safe)

Iterates through all events and invokes each Continuation(null) to signal completion

This optimization provides significant performance benefits for high-throughput scenarios by bypassing the base implementation's lock when message formatting is disabled.

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

coderabbitai

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a112bc4 and 1f94a3c.

📒 Files selected for processing (1)

src/NLog/Targets/NullTarget.cs (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/NLog/Targets/NullTarget.cs (1)

src/NLog/Targets/Target.cs (2)

WriteAsyncThreadSafe (610-623)

WriteAsyncThreadSafe (648-664)

🔇 Additional comments (1)

src/NLog/Targets/NullTarget.cs (1)

36-39: LGTM! Using directives are necessary.

All three new using directives are required for the added functionality:

System.Collections.Generic for IList<AsyncLogEventInfo>

System.Threading for Interlocked operations

NLog.Common for AsyncLogEventInfo type

src/NLog/Targets/NullTarget.cs

sonarqubecloud · 2025-11-01T11:39:04Z

Quality Gate failed

Failed conditions
54.5% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

snakefoot added this to the 6.0.6 milestone Nov 1, 2025

pull-request-size bot added the size/S label Nov 1, 2025

coderabbitai bot reviewed Nov 1, 2025

View reviewed changes

src/NLog/Targets/NullTarget.cs Show resolved Hide resolved

src/NLog/Targets/NullTarget.cs Show resolved Hide resolved

NullTarget - Improve thread concurrency when FormatMessage = false

e3a9dbd

snakefoot force-pushed the NullTarget_ThreadSafe branch from 1f94a3c to e3a9dbd Compare November 1, 2025 11:30

pull-request-size bot added size/M and removed size/S labels Nov 1, 2025

snakefoot merged commit c64c6ae into NLog:dev Nov 1, 2025
4 of 6 checks passed

snakefoot added the performance label Nov 1, 2025

coderabbitai bot mentioned this pull request Nov 8, 2025

Version 6.0.6 #6037

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

NullTarget - Improve thread concurrency when FormatMessage = false#6036

NullTarget - Improve thread concurrency when FormatMessage = false#6036
snakefoot merged 1 commit intoNLog:devfrom
snakefoot:NullTarget_ThreadSafe

snakefoot commented Nov 1, 2025

Uh oh!

coderabbitai bot commented Nov 1, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud bot commented Nov 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Comments

Conversation

snakefoot commented Nov 1, 2025

Uh oh!

coderabbitai bot commented Nov 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud bot commented Nov 1, 2025

Quality Gate failed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai bot commented Nov 1, 2025 •

edited

Loading