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

TestReadFromSquid fails intermittently due to race condition #93

Open
dswarbrick opened this issue Jun 8, 2024 · 1 comment
Open

TestReadFromSquid fails intermittently due to race condition #93

dswarbrick opened this issue Jun 8, 2024 · 1 comment

Comments

@dswarbrick
Copy link
Contributor

dswarbrick commented Jun 8, 2024

Describe the bug
The TestReadFromSquid test case fails intermittently (especially on slower hosts, such as low-powered ARM systems) due to race condition in the goroutine which occasionally tries to read from an uninitialized ch.server (net.Conn).

=== RUN   TestReadFromSquid
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x28 pc=0x36ccb4]

goroutine 3 [running]:
github.com/boynux/squid-exporter/collector.TestReadFromSquid.func1()
        /home/daniel/src/squid-exporter/collector/client_test.go:39 +0x44
created by github.com/boynux/squid-exporter/collector.TestReadFromSquid in goroutine 2
        /home/daniel/src/squid-exporter/collector/client_test.go:37 +0x70
FAIL    github.com/boynux/squid-exporter/collector      0.152s

The ch.server is initialized by the coc.readFromSquid call, so any delays between the goroutine starting and coc.readFromSquid being called will trigger the error.

func TestReadFromSquid(t *testing.T) {
    ch := &mockConnectionHandler{}

    go func() {
        b := make([]byte, 256)
        n, _ := ch.server.Read(b)
        ch.buffer = append(ch.buffer, b[:n]...)

        ch.server.Write(b[n:])
        ch.server.Close()
    }() 

    // Here lies raciness ...

    coc := &CacheObjectClient{
        ch, 
        "", 
        []string{},
    }   
    expected := "GET cache_object://localhost/test HTTP/1.0\r\nHost: localhost\r\nUser-Agent: squidclient/3.5.12\r\nAccept: */*\r\n\r\n"
    coc.readFromSquid("test")

    assert.Equal(t, expected, string(ch.buffer))
}

To Reproduce
Add a small delay just before the coc.readFromSquid("test") line. Failure can be reliably reproduced with delays as small as 5 µs, demonstrating the timing-critical nature of it.

Expected behavior
Test should pass reliably on all systems.

OS (please complete the following information):
Irrelevant / any.

@boynux
Copy link
Owner

boynux commented Jun 22, 2024

Thank you for reporting this and for the analysis. I'll try to get to this as soon as I can. Meanwhile if you already have a fix in mind I'll be happy to review your PR.

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

No branches or pull requests

2 participants