Is there a way to detect following data race code using golangci-lint or other linter??

[raeperd]

Hi big fan of golangci-lint here! thanks a lot for saving me tons of times.

I have question about some hard to find data race error on code.
source is here

package main

import (
        "fmt"
        "time"
)

type RPC struct {
        result int
        done   chan struct{}
}

func (rpc *RPC) compute() {
        time.Sleep(time.Second) // strenuous computation intensifies
        rpc.result = 42
        close(rpc.done)
}

func (RPC) version() int {
        return 1 // never going to need to change this
}

func main() {
        rpc := &RPC{done: make(chan struct{})}

        go rpc.compute()         // kick off computation in the background
        version := rpc.version() // grab some other information while we're waiting
        <-rpc.done               // wait for computation to finish
        result := rpc.result

        fmt.Printf("RPC computation complete, result: %d, version: %d\n", result, version)
}

By fixing func (RPC) version() to func (RPC*) version solves data race which is... very hard to noticed by human reader.
It would be great to catch this by linters

I tried to enable all linters in golangci-lint and many possible linters rules but none of linter catches this data race code.
Am I missing something? or is there any linter that can catch?
Thanks for reading

[ldez]

Hello,

I'm not sure with a static analysis, it is possible to detect this race condition but it can be possible to detect receiver "inconsistency".

[raeperd]

Thanks for reply. is there any linter to check this inconsistency rule? I couldn't find any even outside of golangci-lint supported linters

[ldez]

Some IDEs handle this rule, but I think there is no existing linter on this topic.

[raeperd]

Thanks for reply
I did extra research, I found another reference about this type of err

From Go Wiki: Go Code Review Comments

Don’t mix receiver types. Choose either pointers or struct types for all available methods.
Finally, when in doubt, use a pointer receiver.

Maybe I might try to build new linter to handle this kind of error..

[raeperd]

WIP in https://github.com/raeperd/recvcheck

[ldez]

We will change the plan: I will close my PR and we will use your linter.

But you will need to change your linter to do the same thing as mine: the report must be on the structure (not on the method) and no recommendation of a pointer receiver by default.

[raeperd]

@ldez
Got it. I'll refer to your code to revise it and put you in the reviewer

[k713927]

I have an idea if you want: an improvement of "recv" can be to detect the usage of sync.Mutex as a struct field and in this specific context forces the usage of a pointer receiver.

I will not work on it, so feel free to do a PR on https://github.com/ldez/recv if you want.

@raeperd Hello, for this problem, have you updated it into the plug-in? If not, can I submit pr

[raeperd]

@k713927 Hello.
after my quick test and review, that problem is already solved by go vet: https://github.com/golang/tools/tree/master/go/analysis/passes/copylock
and this check is run by golangci-lint's govet linter
so i don't think your contribution is needed on this specific issue

[k713927]

@k713927 Hello. after my quick test and review, that problem is already solved by go vet: https://github.com/golang/tools/tree/master/go/analysis/passes/copylock and this check is run by golangci-lint's govet linter so i don't think your contribution is needed on this specific issue

Oh, that's right. I hadn't noticed

[walidghallab]

Thanks for sharing the example. I have two comments about this:

1. Would you mind to share where is the race condition? You are saying that it is fixed by making Version ptr receiver, but never mentioned where is the race condition is and if it even exists.
2. The existence of this linter is good, but please note there are some cases where this is not recommended. For example "String()" method that implements fmt.Stringer, it is recommended in a lot of places to have it as a value receiver (regardless of what other methods uses) due to usability, otherwise if you implement it as pointer receiver anyone with a value to the struct (and not a pointer) won't have correct formatting, while value receiver works for both value and pointer.

[raeperd]

@walidghallab
Hi sorry i'm late. I was busy on day job

Example code is completely executable, but data race can be found only when u add -race option

$ go build -race -o app - && •/app
WARNING: DATA RACE
Write at 0x00c000094020 by goroutine 6:
    main. (*RPC) . compute ()
        ~/recvcheck/main.go:15 +0x44
    main.main.gowrap1()
        ~/recvcheck/main.go:26 +0x20
        
Previous read at 0x00c000094020 by main goroutine:
    main-main ()
        ~/recvcheck/main. go:27 +0x120
        
Goroutine 6 (running) created at:
    main main ()
        ~/recvcheck/main.go: 26 +0x114
ニニニニニニニニニニニニニニニ
RPC computation complete, result: 42, version: 1
Found 1 data race(s)

About Stringer interface, if you mix receiver type then it also causes data race.
I could say those recommendations are wrong in terms of concurrent safety.
if you do not mix the type (using value type only), then you are fine

[LandonTClipp]

I'm confused at how this is a data race. The race checker seems to be keying off of the fact that the RPC struct itself is both read from and written to at the same time, but the RPC methods in question do not read or write the same attribute, so how can it be a data race? Version() is not even returning an attribute of the struct itself, it's returning an int literal.

It seems to me like Dave's example is overly contrived and doesn't present a real data race, and the race checker is giving a false positive. Someone tell me I'm wrong, I'm very confused 😅

[raeperd]

@LandonTClipp
I'll help clarify the Go method and struct copy scenario related to potential data races.

In Go, when a method is called on a struct, a copy of the entire struct is passed by default. This copying mechanism can introduce subtle data race conditions, especially when concurrent goroutines are involved. Even if the method doesn't directly use all struct fields, the entire struct is copied, which can create unexpected synchronization challenges.

For example, if you have a struct with some mutable fields and pass it to a method or goroutine, the copy might not prevent concurrent write operations, potentially leading to data races.

To prevent such issues, consider:

• Using pointers to structs
• Implementing proper synchronization (like mutexes)
• Using sync/atomic operations
• Leveraging Go's race detector during development

Key references:

• Go Reddit Discussion on Struct Copying
• Go Race Detector Documentation

and so, this new linter is needed

https://github.com/raeperd/recvcheck

[karelbilek]

I still don't understand how is this a race and why does it matter. version() never reads any data from the struct, so it doesn't really matter that it might have incosistent result and done?

[LandonTClipp]

@raeperd sorry for the late reply, I wish Github was better about notifying me of replies!

Thank you for the explanation. I understand conceptually what you are saying in that making a copy of the struct creates the potential for a data race, but the example we're given does not actually demonstrate a data race because it does not show an attempt to read from and write to the same field of the struct. That's why I was asking for clarification because the example you posted is not a real data race, unless of course I am missing something obvious. Your example demonstrates a potential data race but not a real one.

I don't think I actually disagree or misunderstand anything of what you're saying here. My only confusion was that the example is not a real data race.