Data race between test and source code

[bahrmichael]

See https://github.com/sourcegraph/src-cli/pull/1079 for an example.

Run go test -race ./... on main to reproduce.

The data race seems to come from tests using reflection for mocking, and production code doing other stuff. There appears to be no data race within the production code, only between test and production code.

I traced the first occurrence back to https://github.com/sourcegraph/src-cli/commit/ad1b36ada775f09561eb708657f447c3c7754433#diff-0a603732f0f49068408f58001aca08acd789cc8f3c1abb75db951824fe681ca5.

[camdencheek]

Okay, looked at this a bit and convinced myself that this is only a bug in the test code, and not a real bug.

In short:

• In our generated mock, we call m.Called, reads the fields of all the arguments to record what the function was called with
• One of those fields is an io.Pipe
• We have a separate goroutine that is simultaneously writing to that pipe.
• This is safe because the methods on io.Pipe are safe to call concurrently, even if it's not safe to use reflection to read the struct fields
• The error was from an atomic operation on an int32, which was not accessed with an atomic load in testify

I do not think this is high priority since I'm pretty confident this is only a test bug, but probably the best solution here would be to change the multipart writer to instead be a multi-part reader that pulls on Read() rather than pushes to the pipe. It's cleaner anyways (does not require any goroutines).

[varungandhi-src]

but probably the best solution here would be to change the multipart writer to instead be a multi-part reader that pulls on Read() rather than pushes to the pipe

I don't quite understand this, but it seems like there is a semantic error in uploadFile, where the writer's Close operation is not guaranteed to have been complete before the function returns. Is it technically possible for the Close operation to return an error, and if so, is it OK to discard that error?

[varungandhi-src]

This was flagged as an issue against the testify repo here: https://github.com/stretchr/testify/issues/1597

[varungandhi-src]

Potential workaround PR to make CI green by disabling race detection for that one test: https://github.com/sourcegraph/src-cli/pull/1082

[camdencheek]

the writer's Close operation is not guaranteed to have been complete before the function returns

Is it not? Calling Close() is what closes the request body, so the request cannot complete until either: 1) Close() is called, closing the request body and allowing the request to complete, or 2) the request errored, in which case CloseWithError will be called possibly after function return, but it doesn't really matter because there are no more readers of the pipe anyways.

[camdencheek]

Good find on the testify issue! Disabling race detection for this one seems okay to me 👍

[varungandhi-src]

Calling Close() is what closes the request body

Ah, OK, I didn't quite realize this. I thought the signature of the race condition indicated that the writer closing was concurrent with the mock test assertion function being called. You wrote this earlier:

We have a separate goroutine that is simultaneously writing to that pipe

You used the word simultaenously here.

However, if Close() happens-before the function return and the function return happens-before the call to m.Called, then it seems like the race detection logic in the Go tooling is wrong, given that the atomic write is guaranteed to happen-before the non-atomic read?