Conversation
|
This is an interesting race! The issue is that we are using named return variables in Super simple example that triggers the same race (and shows what the problem is... kind of): I suspect this has never been a problem for us (although I'd want to check the logs to be sure) because |
|
First, thanks for fixing this bug, @jsha, and thanks @rolandshoemaker for the explanation. I believe I introduced this and I had no idea this was something to watch for! As far as the fix is concerned: Just as I overlooked it the first time, I think 1 year from now I'd likely revert this change as part of cleaning up code. So I think something else or additional should be done to prevent this from happening. e.g. Stop using named return values in this function and/or add a comment saying that the goroutine is given a complete copy of the cert DER to avoid a data race or something. |
@briansmith Thanks for bringing that up! I agree that there should at least be a comment here. I wish I thought of that during review :-) I think we've put a hold on any new code using named return values so its probably worthwhile to clean up the existing occurrences. |
|
Yep, I agree we should clean this up more thoroughly by avoiding named return values: #3019. Excellent sleuthing work @rolandshoemaker, thanks for writing it up! |
I can reliably reproduce a data race by doing the following things:
cmd.Clock()so I can fake dates in the futureThe race detector output is below. However, I'm confused about the output; it claims that there's a write on a line that only has a
returnstatement on it. I poked at the code a little bit, and this diff appears to solve it: forcing a copy ofcert.DERbefore forking off a goroutine. However, I'm confused about why this would be an issue. My understanding of how Go handles function-scoped variables is that they'll be retains as long as they need to be based on reference counting. What are your thoughts? I'd like to understand this better!