On Sat, Sep 11, 2021 at 6:49 PM Michael Dwyer <michael.k.dw...@gmail.com>
wrote:
> Kurtis,
>
> I realized this question would arise.
> Presently working with an existing code base.
> The code presented in the example is a close approximation to what
> actually exists, the only thing changed were the names.
> Additionally, I checked the package and the call order is SomeFunc() ->
> Refresh().
> So there will be goroutines that will be calling SomFunc(), which will
> then call Refresh().
> Looking at the code I can confirm that Refresh() is not called directly
> from this package.
>
What you wrote is slightly confusing since I think you meant "not called
directly outside this package". In which case the first thing to do is
change "Refresh" to "refresh" since it doesn't need to be public. If that
breaks the build then, obviously, there are other callers you aren't aware
of. However, even if that doesn't break the build I question whether the
Refresh() method is even warranted given that it is trivial in your example.
> The problem I face is working with the existing code base, identify
> problems, viz., Data Race, look at the code figure out what the best
> solution to fix the proximate problem.
>
> I do agree with your assessment as to why code would be written in the
> manner, in which it now exists.
> I do not, at this time, have an answer to this question.
>
There can be more than one explanation and they are not mutually exclusive.
For example, the original code might have been written by someone
inexperienced such as a NCG (new college grad). Perhaps the code was
translated from an existing implementation written in a different language
(e.g., C/C++) without considering whether a literal translation was
appropriate.
> Working with the authors is difficult at best.
> Basically, working alone, with assistance, when really needed, from this
> group.
>
That is unfortunate since the authors of the code should be able to explain
why it is structured in this fashion. Are there any code comments that shed
light on how the "Foo" structure is intended to be used? Specifically, the
rules for safe concurrent access? What about comments on methods like
"Refresh"?
> Presently, taking the existing code, and provide the best solution for
> problems without changing present structure.
> Which is why I reached out, because the way the code is presently
> structured, trying to figure out if my proposed solution is workable.
>
> I realize this is not ideal code, could be, should be written cleaner.
>
> My question then is this, using the template of the hypothetical code
> presented, knowing that there are going to be several goroutines calling
> this code,
> will my proposed solution address the Data Race problem sufficiently?
> Have I missed any potential edge cases, that I have not thought through?
>
Was it your intent to introduce a package level sync.Mutex? If so you're
protecting code, not data. Which is always a bad idea (modulo esoteric
situations that don't seem applicable to your problem). You need to add a
sync.Mutex member to the Foo struct to protect data, not code. In which
case you're making a substantive change. Most importantly, you need to use
the mutex you've added every single place that references Foo.bar. Too,
it's a red flag something isn't quite right to have a mutex protect an
individual member of a structure rather than the entire structure. Yes,
there are exceptions to that generalization but it should be reflected in
the documentation of the data structure. The fact that your SomeFunc()
method has a "foo.bar == nil" test is another red flag. Why isn't that
struct member already initialized when the SomeFunc() method is executed?
Does every other place that uses "Foo.bar" have an equivalent test? If not,
why not?
Finally, the Go race detector is amazing but it does not guarantee to
detect all data races. Given what you've told us so far, and the fact it
did detect one race, I would be worried there are more problems of this
nature in the code that it has not (yet) detected.
On Saturday, September 11, 2021 at 8:42:36 PM UTC-4 Kurtis Rader wrote:
> On Sat, Sep 11, 2021 at 5:01 PM Michael Dwyer <michael...@gmail.com>
>> wrote:
>>
>>> Presently working on a Data Race and have some questions on how to
>>> proceed.
>>> The code example is a hypothetical mock up of the actual code under
>>> review, but is a fair representation.
>>>
>>> The struct used by both functions is named Foo.
>>> In the hypothetical, SomeFunc() reads variable from foo.bar, checking
>>> for nil,
>>> if nil SomeFunc() calls a Foo method Refresh().
>>> Foo method Refresh() calls function DoSomeCalc() assigned to foo.bar and
>>> returned back to method SomeFunc()
>>>
>>> The race detector flags the read of foo.bar in method SomeFunc() in the
>>> if statement,
>>> and flags the write of foo.bar in method Refresh().
>>>
>>> The problem I see is that in method SomeFunc() the if statement does a
>>> read,
>>> but it too also does an assignment to foo.bar.
>>>
>>> My initial thought is to add a sync.Lock() at the beginning of method
>>> SomeFunc(),
>>> followed by a defer sync.UnLock(),
>>> that will be held until method SomeFunc() exists.
>>> I state this because method SomeFunc() also does an assignment to
>>> variable foo.bar.
>>> So, a sync.RLock() would be insufficient to protect variable foo.bar
>>>
>>> My only concern is the call to method Refresh(), which also writes to
>>> foo.bar.
>>>
>>> If my understanding is correct, this should work, because the lock is
>>> initiated in method SomeFunc(), held to the exit of the method.
>>> With the call to method Refresh() occurring within method SomeFunc() the
>>> lock should protect variable foo.bar from other goroutines trying to update
>>> this variable.
>>>
>>> Asking this question to confirm my thought process is correct.
>>> If there is an edge case I missed, please advise.
>>>
>>> func (foo *Foo) SomeFunc() fields.SomeVariable {
>>> if foo.bar == nil { <=== Race detector flags
>>> this read
>>> foo.bar = foo.ReFresh()
>>> }
>>> return foo.bar
>>> }
>>>
>>> func (foo *Foo) ReFresh() fields.SomeVariable {
>>> foo.bar = DoSomeCalc(foo) <=== Race detector flags
>>> this write
>>> return foo.bar
>>> }
>>>
>>
>> I know this is psuedo-code meant to illustrate the relevant aspect of
>> your real code but I don't understand why you're assigning to foo.bar in
>> the ReFresh() method as well as where you invoke this method in SomeFunc().
>> If that isn't a mistake introduced while writing the simplified example it
>> suggests there are other problems beyond the data race. It also seems like
>> you've omitted some important details about the real program since the race
>> implies that ReFresh() is called from a goroutine other than the one
>> executing SomeFunc().
>>
>> --
>> Kurtis Rader
>> Caretaker of the exceptional canines Junior and Hank
>>
>
--
Kurtis Rader
Caretaker of the exceptional canines Junior and Hank
--
You received this message because you are subscribed to the Google Groups
"golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to golang-nuts+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/golang-nuts/CABx2%3DD--R%2B8YoS5cwHtRycMOVJjzZV6iB1G3CRRR5G8kj95yzQ%40mail.gmail.com.
