fix: add write mutex for log writing to file#943
fix: add write mutex for log writing to file#943ukpratik wants to merge 3 commits intosipeed:mainfrom
Conversation
nikolasdehor
left a comment
nikolasdehor
left a comment
There was a problem hiding this comment.
The intent is correct -- adding a mutex to prevent interleaved log writes, especially on Windows. However, there are some issues with the implementation:
-
Lock ordering concern: The code acquires mu.RLock() (global read lock) then logger.writeMu.Lock() (per-logger write lock). This nested locking is safe here since mu is always acquired first, but the mu.RLock/RUnlock now wraps only the file-writing section and not the console output. Before this change, there was no locking around the file write at all. The mu.RLock/RUnlock placement seems intentional (to prevent the file from being closed/swapped while writing), which is a good defensive improvement.
-
WriteString allocation: Changed from file.Write(append(jsonData, newline)) to file.WriteString(string(jsonData) + newline). This creates an extra string allocation via string concatenation. The original Write(append(jsonData, newline)) was more efficient. Consider instead:
logger.writeMu.Lock()
_, err = fileptr.Write(append(jsonData, newlineByte))
logger.writeMu.Unlock() -
The test is good -- 100 concurrent goroutines writing and verifying all lines appear. Running with -race is the right validation.
-
Minor: the comment says 'Although POSIX systems typically guarantee atomicity for single write() syscalls' -- this is approximately correct for writes under PIPE_BUF (4096 bytes), but log lines can be larger. The mutex is the right call regardless.
Overall the fix is sound, just the WriteString allocation could be avoided. Not a blocker.
|
|
|
|
Closing this PR as it may no longer be needed after #1239 (refactor logger to zerolog) was merged. The zerolog-based file logging implementation should handle concurrent writes properly. If there are still concurrency issues with the new implementation, please open a new PR with the fix based on the current main branch. |
4 participants
📝 Description
Add per-logger writeMu mutex to ensure safe concurrent file writes
🗣️ Type of Change
🤖 AI Code Generation
Note: Test was fully written by AI and then that test was verified by Human
🔗 Related Issue
Resubmitting -> #191 : fix: minor fix in logger file writing
Fix indirectly relates to Roadmap Item #346
📚 Technical Context (Skip for Docs)
🧪 Test Environment
📸 Evidence (Optional)
Click to view Logs/Screenshots
$ go test -race ./pkg/logger -run TestConcurrentFileLogging -count=1☑️ Checklist