XmlLoggingConfiguration - Protect against double dispose of AutoReloadConfigFileWatcher #6006
Conversation
WalkthroughTightens and reorders disposal and stop conditions for XmlLoggingConfiguration's auto-reload watcher: stricter check before stopping, streamlined timer disposal, and a re-entrancy-guarded Dispose that captures the timer, clears state under lock, then unsubscribes and disposes after the lock to reduce races. (49 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Config as XmlLoggingConfiguration
participant Watcher as AutoReloadConfigFileWatcher
participant FS as FileSystemWatcher
participant Timer as ReloadTimer
rect rgba(230,240,255,0.45)
Caller->>Config: OnConfigurationAssigned()
alt watcher != null && watcher not disposed
Config->>Watcher: Stop()/Dispose()
else
Note right of Config: no stop/dispose
end
end
rect rgba(240,255,230,0.45)
Caller->>Watcher: Dispose()
alt first call (not disposing)
Watcher->>Watcher: capture localReloadTimer
Watcher->>Watcher: lock(this)\nset _isDisposing = true\nclear fields (e.g., _reloadTimer)
Watcher-->>FS: Unsubscribe FileChanged (after lock)
Watcher-->>Timer: Dispose captured timer (after lock)
else re-entrant call
Watcher->>Watcher: return early (guard)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/NLog/Config/XmlLoggingConfiguration.cs (1)
⏰ 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)
🔇 Additional comments (3)
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. Comment |
75e7278 to
3f3b473
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/NLog/Config/XmlLoggingConfiguration.cs (1)
647-650: Consider consolidating duplicate disposal checksThe
_isDisposingcheck appears twice at the beginning of this method (lines 647 and 658). While the second check after acquiring the lock is necessary for thread safety, the first check seems redundant since the timer callback should only execute if the timer hasn't been disposed yet.Consider removing the first check to simplify the code:
private void ReloadTimer(object state) { - if (_isDisposing) - { - return; //timer was disposed already. - } - LoggingConfiguration oldConfig = (LoggingConfiguration)state;The lock-protected check on line 658 provides sufficient protection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/NLog/Config/XmlLoggingConfiguration.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/NLog/Config/XmlLoggingConfiguration.cs (2)
src/NLog/Internal/TimeoutContinuation.cs (1)
Dispose(85-88)src/NLog/Internal/MultiFileWatcher.cs (1)
Dispose(71-75)
⏰ 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 (2)
src/NLog/Config/XmlLoggingConfiguration.cs (2)
259-259: Good defensive check against disposed watchersThe additional
!fileWatcher.IsDisposedcheck prevents unnecessary disposal attempts on already disposed watchers. This is a solid defensive programming practice.
722-735: Well-implemented double-dispose protection with improved thread safetyThe refactored
Disposemethod introduces several important improvements:
- Captures the timer reference before entering the lock to minimize lock scope
- Adds a re-entrancy guard with
_isDisposingcheck to prevent double disposal- Properly sequences operations: state cleanup under lock, then disposal outside the lock to avoid potential deadlocks
- Unsubscribes from events and disposes resources after releasing the lock
This pattern effectively prevents race conditions and ensures clean resource cleanup.
…dConfigFileWatcher
|
No description provided.