log: fix data race in RotatingFileHandler#17665
log: fix data race in RotatingFileHandler#17665simplyzhao wants to merge 4 commits intoethereum:masterfrom
Conversation
| // the values of any lazy fn to the result of its execution | ||
| hadErr := false | ||
| for i := 1; i < len(r.Ctx); i += 2 { | ||
| lz, ok := r.Ctx[i].(Lazy) |
There was a problem hiding this comment.
Nitpick, maybe merge the two, like if lz, ok := r.Ctx[i].(Lazy); ok {
Otherwise LGTM, thanks!
There was a problem hiding this comment.
Actually, I didn't change code here, just extract code from the original LazyHandler so that it can be re-used.
There was a problem hiding this comment.
I see, it just caught my eye :) Could you please do this change in order to keep the code more compact?
|
LGTM, thanks! |
|
|
||
| return FuncHandler(func(r *Record) error { | ||
| // lazy evaluate, this needs no lock. | ||
| lazyEvaluateRecord(r) |
There was a problem hiding this comment.
This is a new addition, right? It wasn't mentioned in the PR description. What was the result before versus after applying this PR?
There was a problem hiding this comment.
This is not a new addition. Try to fix a data race in the original code which used StreamHandler here.
There was a problem hiding this comment.
Basically, I'm trying to achieve the same effect of StreamHandler except move the lock out so that it can protect the count and w of the RotatingFileHandler
|
I don't think all this complexity is needed/warranted. The data race can (I think) be solved by declaring the mutex as you did, but instead of lock/defer-unlock, unlock it before the |
|
I pushed some changes on top. @karalabe PTAL if it's in line with what you thought, and @simplyzhao PTAL if you think it there's any error in it. I pushed on top of your commits, to not steal the credit from you |
|
I'm unsure this PR fixes the issue now. We protect reading the count, but nowhere do we protect updating it. |
fjl
dismissed
their stale review
Didn't see @karalabe's comment. We need to rework this change.
|
We have decided to remove |
6 participants
The
StreamHandler(which containsSyncHandler) can only protect the actualLogoperation. In RotatingFileHandler we need to protect thecountandwincountingWriter.