Skip to content

Refactor Fluent Logger for Improved Thread Safety and Error Handling#130

Merged
cosmo0920 merged 9 commits intofluent:masterfrom
aaithal:refactor_potential_races
May 19, 2025
Merged

Refactor Fluent Logger for Improved Thread Safety and Error Handling#130
cosmo0920 merged 9 commits intofluent:masterfrom
aaithal:refactor_potential_races

Conversation

@aaithal
Copy link
Copy Markdown
Contributor

@aaithal aaithal commented May 8, 2025

Summary

This PR refactors the Fluent logger implementation to improve certain aspects of thread safety and error handling.

Key Changes

1. Thread Safety
  • Introduced thread-safe wrapper methods with sync prefix that handle their own locking
  • Improved mutex usage patterns to prevent potential deadlocks
  • Added proper double-checking of the closed flag after acquiring locks
2. Error Handling
  • Added proper error handling for SetWriteDeadline() and SetReadDeadline()
  • Implemented context cancellation checks throughout the codebase
  • Used Go 1.13+ error wrapping with %w for better error context preservation
  • Added more specific error messages with consistent prefixes
3. Connection Management
  • Created syncClose() method to safely close connections
  • Added syncConnectWithRetry() to handle connection establishment in a thread-safe manner
  • Improved connection state checking to prevent operations on nil connections
4. Read Timeout Support
  • Added support for read timeouts when reading acknowledgments
  • Added a new ReadTimeout configuration option with appropriate defaults
  • Set read deadlines on connections
5. Context Propagation
  • Ensured context is properly propagated to all network operations
  • Added context cancellation checks at critical points to allow early exit
  • Improved handling of context cancellation in the run() goroutine

Testing

All existing tests pass with these changes. The refactoring maintains backward compatibility while improving the robustness of the code.

aaithal added 6 commits May 5, 2025 17:08
@aaithal
fluent: remove stopRunning channel.
200b3e5 
The channel and its usage is redundant since we already wire Fluent
object with a cancellable context. Removed stopRunning to use the
context instead.

Instead of waiting for stopRunning in run(), we can just wait for
context to be done as well.

Signed-off-by: Anirudh Aithal <aithal@amazon.com>
@aaithal
fluent: pending channel thread saftey test
f1b233b 
Added a new test to ensure that pending channel is accessed in a
thread safe manner. The code can be simplified by removing the
pendingMutex lock. There's a risk in this codebase wrt how muconn
and pendingMutex are acquired and released in different orders in
different methods.

This test is a precursor to the change to remove the pendingMutex
to ensure that nothing is broken.

Signed-off-by: Anirudh Aithal <aithal@amazon.com>
@aaithal
fluent: make connection closed variable thread-safe
10e7785 
Changed the "closed" variable to be an atomic int. There is a race condition b/w
appendBuffer and Close() method wrt how this variable is accessed. This could be
an atomic.Bool, but the CI/CD config uses golang version 1.17. Since atomic.Bool
is supported 1.19 onwards, using that would fail tests and build.

Data race trace without this fix (using `go test -race`):
==================
WARNING: DATA RACE
Write at 0x00c0003324e0 by goroutine 95:
  github.com/fluent/fluent-logger-golang/fluent.(*Fluent).close()
      /Users/aithal/workspace/src/github.com/aaithal/fluent-logger-golang/fluent/fluent.go:426 +0x13c
  github.com/fluent/fluent-logger-golang/fluent.(*Fluent).Close()
      /Users/aithal/workspace/src/github.com/aaithal/fluent-logger-golang/fluent/fluent.go:393 +0x108
  github.com/fluent/fluent-logger-golang/fluent.TestCloseWhileWaitingForAckResponse.func1()
      /Users/aithal/workspace/src/github.com/aaithal/fluent-logger-golang/fluent/fluent_test.go:737 +0x2c
  github.com/fluent/fluent-logger-golang/fluent.timeout.func1()
      /Users/aithal/workspace/src/github.com/aaithal/fluent-logger-golang/fluent/fluent_test.go:557 +0x38

Previous read at 0x00c0003324e0 by goroutine 94:
  github.com/fluent/fluent-logger-golang/fluent.(*Fluent).write()
      /Users/aithal/workspace/src/github.com/aaithal/fluent-logger-golang/fluent/fluent.go:619 +0x1ac
  github.com/fluent/fluent-logger-golang/fluent.(*Fluent).writeWithRetry()
      /Users/aithal/workspace/src/github.com/aaithal/fluent-logger-golang/fluent/fluent.go:553 +0x78
  github.com/fluent/fluent-logger-golang/fluent.(*Fluent).run()
      /Users/aithal/workspace/src/github.com/aaithal/fluent-logger-golang/fluent/fluent.go:527 +0x1d0
  github.com/fluent/fluent-logger-golang/fluent.newWithDialer.gowrap1()
      /Users/aithal/workspace/src/github.com/aaithal/fluent-logger-golang/fluent/fluent.go:184 +0x4c

Goroutine 95 (running) created at:
  github.com/fluent/fluent-logger-golang/fluent.timeout()
      /Users/aithal/workspace/src/github.com/aaithal/fluent-logger-golang/fluent/fluent_test.go:556 +0xe8
  github.com/fluent/fluent-logger-golang/fluent.TestCloseWhileWaitingForAckResponse()
      /Users/aithal/workspace/src/github.com/aaithal/fluent-logger-golang/fluent/fluent_test.go:736 +0x444
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.24.2/libexec/src/testing/testing.go:1792 +0x180
  testing.(*T).Run.gowrap1()
      /opt/homebrew/Cellar/go/1.24.2/libexec/src/testing/testing.go:1851 +0x40

Goroutine 94 (running) created at:
  github.com/fluent/fluent-logger-golang/fluent.newWithDialer()
      /Users/aithal/workspace/src/github.com/aaithal/fluent-logger-golang/fluent/fluent.go:184 +0x368
  github.com/fluent/fluent-logger-golang/fluent.TestCloseWhileWaitingForAckResponse()
      /Users/aithal/workspace/src/github.com/aaithal/fluent-logger-golang/fluent/fluent_test.go:722 +0xd8
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.24.2/libexec/src/testing/testing.go:1792 +0x180
  testing.(*T).Run.gowrap1()
      /opt/homebrew/Cellar/go/1.24.2/libexec/src/testing/testing.go:1851 +0x40
==================
[2025-05-06T08:42:08-07:00] Discarding queued events...
    testing.go:1490: race detected during execution of test

Signed-off-by: Anirudh Aithal <aithal@amazon.com>
@aaithal
fluent: refactor code for muconn lock acquisition
f81b4e6 
Refactored the code to follow the pattern of releasing the mutex lock
in a defer block as much as possible for the muconn lock. This should make
the code more maintainable, avoiding issues with not accidentally releasing
the lock.

Signed-off-by: Anirudh Aithal <aithal@amazon.com>
@aaithal
fluent: check if context is cancelled before performing I/O on the co…
11bef51 
…nnection

There are a number of places in the code where we do not check if the context
is cancelled before reading from or writing to the connection. This commit adds
those checks.

Signed-off-by: Anirudh Aithal <aithal@amazon.com>

cr: https://code.amazon.com/reviews/CR-194284782
Signed-off-by: Anirudh Aithal <aithal@amazon.com>
@aaithal
fluent: add read timeout config
1f759a4 
This can be used by clients to exit early when the connection is
unresponsive while reading acks from the server.

Signed-off-by: Anirudh Aithal <aithal@amazon.com>
@aaithal aaithal force-pushed the refactor_potential_races branch from 64e3b2b to 1f759a4 Compare May 8, 2025 22:46
cosmo0920
cosmo0920 reviewed May 19, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@cosmo0920 cosmo0920 left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Plus, I tested with race detector like as:
go test -v -race -cover -covermode=atomic ./fluent

There is no error from race detector.
The current master is still failing with that.

@cosmo0920
Copy link
Copy Markdown
Contributor

cosmo0920 commented May 19, 2025

Could you rebase off current master? I modified to run CI tasks correctly. So, we wanted to check CI results before merging.

cosmo0920 added 3 commits May 19, 2025 19:25
@cosmo0920
Merge branch 'master' into refactor_potential_races
1429197
@cosmo0920
test: Follow changes of testing framwwork
ff8e422 
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
@cosmo0920
workflows: Use race detector on test
6a76acc 
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
@cosmo0920 cosmo0920 merged commit 38df75e into fluent:master May 19, 2025
7 checks passed
@cosmo0920 cosmo0920 mentioned this pull request May 19, 2025
Merged
@singholt singholt mentioned this pull request May 19, 2025
Closed
@aaithal
Copy link
Copy Markdown
Contributor Author

aaithal commented Jun 19, 2025

Thank you for merging this PR and apologies for not getting back on this sooner. I taken a break for a few weeks. Appreciate you merging these changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cosmo0920 cosmo0920 cosmo0920 approved these changes

Assignees

@cosmo0920 cosmo0920

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@aaithal @cosmo0920