Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix new writer in logx #4311

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

fix new writer in logx #4311

wants to merge 5 commits into from

Conversation

marsmay
Copy link

@marsmay marsmay commented Aug 9, 2024

NewWriter 方法用于自定义 logx 的 Writer,例如:
NewWriter method is used to customize logx's Writer, e.g.:

logx.SetWriter(logx.NewWriter(w))

但当前的 logx.NewWriter 使用 log.New 和 logx.newLogWriter 对传入的自定义 writer 进行了包装;
实际调用时,调用路径为,log.Print -> log.output -> writer.Write;
这会导致一系列问题,因为 log 库中使用了 sync.Pool 产生 *[]byte 用于拼接字符串;
如果自定义 writer 是同步写入,则可能不会出现问题;
如果自定义 writer 是异步写入,如使用 chan 来实现并发安全,这时实际传入 writer.Write 的 []byte 可能具有同一地址,造成写入重复内容;
并且 logx.newLogWriter 函数还忽略了自定义 writer 的 Close 方法;
通过修改 logx.NewWriter 方法,直接传入 io.WriteCloser,可以避免这些问题;
在实际测试中也是符合预期的;

When it is actually called, the path is log.Print -> log.output -> writer.Write;
This will lead to some problems, the std log library uses sync.Pool to generate *[]byte for string splicing;
If the custom writer writes synchronously, there may not be a problem;
But if the custom writer writes asynchronously, e.g., using chan for concurrency safety, the []byte that is actually passed to writer.Write may have the same address, resulting in duplicate content being written;
And logx.newLogWriter function also ignores the Close method of the custom writer;
These problems can be avoided by modifying the logx.NewWriter method and passing in io.WriteCloser directly;
It also meets the expectation in the actual test;

@kevwan
Copy link
Contributor

kevwan commented Aug 10, 2024

Thanks for your contribution!

But this is a breaking change. And please add unit tests.

@marsmay
Copy link
Author

marsmay commented Aug 11, 2024

Add NewAsyncWriter function to avoid affecting previous code.
Add unit test for NewAsyncWriter.

Copy link

codecov bot commented Aug 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.04%. Comparing base (8690859) to head (09e7c39).
Report is 129 commits behind head on master.

Additional details and impacted files
Files Coverage Δ
core/logx/writer.go 97.06% <100.00%> (+1.53%) ⬆️

... and 254 files with indirect coverage changes

@kevwan
Copy link
Contributor

kevwan commented Aug 27, 2024

Merge this PR later. Let's figure out if it's possible to fix the problem with the same method NewWriter.

Would you please give a simple example to reproduce the problem?

@marsmay
Copy link
Author

marsmay commented Aug 28, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants