Data Race Issue

[dcramer]

Let me start by saying, this might be entirely my fault, and I barely understand anything about Go.

I'm attempting to convert a test from using a pretty ugly chunk of httptest code to httpmock:

https://github.com/dropbox/changes-client/compare/handle-abortions...httpmock

However, upon doing this we hit a race which only presents itself with httpmock:

==================
WARNING: DATA RACE
Write by goroutine 7:
  github.com/jarcoal/httpmock.Deactivate()
      /home/vagrant/src/github.com/jarcoal/httpmock/transport.go:139 +0x51
  github.com/jarcoal/httpmock.DeactivateAndReset()
      /home/vagrant/src/github.com/jarcoal/httpmock/transport.go:150 +0x2b
  github.com/dropbox/changes-client/engine.TestCompleteFlow()
      /home/vagrant/src/github.com/dropbox/changes-client/engine/engine_test.go:296 +0x1b4c
  testing.tRunner()
      /usr/local/go/src/pkg/testing/testing.go:422 +0x10f

Previous read by goroutine 11:
  net/http.(*Client).send()
      /usr/local/go/src/pkg/net/http/client.go:118 +0x39e
  net/http.(*Client).doFollowingRedirects()
      /usr/local/go/src/pkg/net/http/client.go:343 +0xd31
  net/http.(*Client).Get()
      /usr/local/go/src/pkg/net/http/client.go:275 +0xcd
  net/http.Get()
      /usr/local/go/src/pkg/net/http/client.go:252 +0x6e
  github.com/dropbox/changes-client/engine.(*UpstreamMonitor).fetchJobStep()
      /home/vagrant/src/github.com/dropbox/changes-client/engine/upstream_monitor.go:55 +0x151
  github.com/dropbox/changes-client/engine.(*UpstreamMonitor).WaitUntilAbort()
      /home/vagrant/src/github.com/dropbox/changes-client/engine/upstream_monitor.go:33 +0x6c
  github.com/dropbox/changes-client/engine.func·005()
      /home/vagrant/src/github.com/dropbox/changes-client/engine/engine.go:213 +0x95

Goroutine 7 (running) created at:
  testing.RunTests()
      /usr/local/go/src/pkg/testing/testing.go:504 +0xb46
  testing.Main()
      /usr/local/go/src/pkg/testing/testing.go:435 +0xa2
  main.main()
      github.com/dropbox/changes-client/engine/_test/_testmain.go:47 +0xdc

Goroutine 11 (running) created at:
  github.com/dropbox/changes-client/engine.(*Engine).runBuildPlan()
      /home/vagrant/src/github.com/dropbox/changes-client/engine/engine.go:217 +0x8c9
  github.com/dropbox/changes-client/engine.(*Engine).Run()
      /home/vagrant/src/github.com/dropbox/changes-client/engine/engine.go:75 +0x1e5
  github.com/dropbox/changes-client/engine.RunBuildPlan()
      /home/vagrant/src/github.com/dropbox/changes-client/engine/engine.go:59 +0x1a8
  github.com/dropbox/changes-client/engine.TestCompleteFlow()
      /home/vagrant/src/github.com/dropbox/changes-client/engine/engine_test.go:196 +0xb7b
  testing.tRunner()
      /usr/local/go/src/pkg/testing/testing.go:422 +0x10f
==================
==================
WARNING: DATA RACE
Write by goroutine 7:
  github.com/jarcoal/httpmock.Reset()
      /home/vagrant/src/github.com/jarcoal/httpmock/transport.go:144 +0x4e
  github.com/jarcoal/httpmock.DeactivateAndReset()
      /home/vagrant/src/github.com/jarcoal/httpmock/transport.go:151 +0x30
  github.com/dropbox/changes-client/engine.TestCompleteFlow()
      /home/vagrant/src/github.com/dropbox/changes-client/engine/engine_test.go:296 +0x1b4c
  testing.tRunner()
      /usr/local/go/src/pkg/testing/testing.go:422 +0x10f

Previous read by goroutine 11:
  github.com/jarcoal/httpmock.(*MockTransport).responderForKey()
      /home/vagrant/src/github.com/jarcoal/httpmock/transport.go:63 +0x43
  github.com/jarcoal/httpmock.(*MockTransport).RoundTrip()
      /home/vagrant/src/github.com/jarcoal/httpmock/transport.go:41 +0x10e
  net/http.send()
      /usr/local/go/src/pkg/net/http/client.go:195 +0x62d
  net/http.(*Client).send()
      /usr/local/go/src/pkg/net/http/client.go:118 +0x200
  net/http.(*Client).doFollowingRedirects()
      /usr/local/go/src/pkg/net/http/client.go:343 +0xd31
  net/http.(*Client).Get()
      /usr/local/go/src/pkg/net/http/client.go:275 +0xcd
  net/http.Get()
      /usr/local/go/src/pkg/net/http/client.go:252 +0x6e
  github.com/dropbox/changes-client/engine.(*UpstreamMonitor).fetchJobStep()
      /home/vagrant/src/github.com/dropbox/changes-client/engine/upstream_monitor.go:55 +0x151
  github.com/dropbox/changes-client/engine.(*UpstreamMonitor).WaitUntilAbort()
      /home/vagrant/src/github.com/dropbox/changes-client/engine/upstream_monitor.go:33 +0x6c
  github.com/dropbox/changes-client/engine.func·005()
      /home/vagrant/src/github.com/dropbox/changes-client/engine/engine.go:213 +0x95

Goroutine 7 (running) created at:
  testing.RunTests()
      /usr/local/go/src/pkg/testing/testing.go:504 +0xb46
  testing.Main()
      /usr/local/go/src/pkg/testing/testing.go:435 +0xa2
  main.main()
      github.com/dropbox/changes-client/engine/_test/_testmain.go:47 +0xdc

Goroutine 11 (running) created at:
  github.com/dropbox/changes-client/engine.(*Engine).runBuildPlan()
      /home/vagrant/src/github.com/dropbox/changes-client/engine/engine.go:217 +0x8c9
  github.com/dropbox/changes-client/engine.(*Engine).Run()
      /home/vagrant/src/github.com/dropbox/changes-client/engine/engine.go:75 +0x1e5
  github.com/dropbox/changes-client/engine.RunBuildPlan()
      /home/vagrant/src/github.com/dropbox/changes-client/engine/engine.go:59 +0x1a8
  github.com/dropbox/changes-client/engine.TestCompleteFlow()
      /home/vagrant/src/github.com/dropbox/changes-client/engine/engine_test.go:196 +0xb7b
  testing.tRunner()
      /usr/local/go/src/pkg/testing/testing.go:422 +0x10f
==================

[jarcoal]

Are you running the tests with T.parallel by chance?

When you call httpmock.Activate() a non thread safe operation occurs where the http package's DefaultTransport is overwritten by httpmock's MockTransport. Relevant code. This works fine unless you're running tests in parallel.

If you would like to avoid this, then you'll need to have some code pass a Transport to your HTTP clients (rather than letting them use DefaultTransport). This will let you give the clients the MockTransport instead when running tests.

[dcramer]

@jarcoal not using T.parallel, just running with -race

[panamafrancis]

I experienced something similar to this while trying to mock one or two calls at a great height where many calls i didn't so much care about were being made. My solution to this was to make MockTransport a wrapper around the original http.DefaultTransport, then ensuring that its RoundTrip() method got called instead of ConnectionFailure().

I think in some cases the ConnectionFailure() method can cause havoc, but i'd need to investigate further.

I didn't stash my code, sorry, but maybe it'll help you find a fix.

[SchumacherFM]

This project seems dead ... To fix the race condition add a sync.Mutex to the MockTransport struct ;-)

[jarcoal]

Yeah I'm not really writing Go anymore so haven't been working on this. If anyone wants to be added as a collaborator, let me know.

[SchumacherFM]

Pick me Pick me Pick me!

[jarcoal]

Hey @SchumacherFM I made you a collaborator, thanks for helping out.

[SchumacherFM]

I have an initial idea quick and dirty implemented https://github.com/SchumacherFM/httpmock/blob/try-to-remove-race/transport.go#L153

What is missing is to make a process wait while another process holds the http.DefaultTransport lock ...

Little bit late my answer, stuck on other projects :-(

[Pomyk]

I don't think this can be solved in the general case. Even if you use locks in Activate/Deactivate the client can still use the transport (in anothor goroutine) and there is a race. The best solution is to use custom client and pass transport as a param so you can mock it.

[daohoangson]

The fix in abbf1543e32e3d8a74ab639e37d9b097178a6924 breaks my use case. Basically my app works as a proxy and in my tests, we send a request to our web server then the app will make another request to a mocked address:

1. test.go makes request GET http://localhost:9999/some/path
2. server.go receives that request and makes request GET http://mocked.com/some/path

I have dig around and it looks like the 1st request is still holding the lock while the 2nd request reaches responderForKey. As a workaround, we now use a new &http.Client{} for requests made by test.go. Is that recommended?

    var tmpClient = &http.Client{
        Transport: httpmock.InitialTransport,
    }

[Pomyk]

I think this is ok.

This reminded me of: https://xkcd.com/1172/ :)

[maxatome]

@daohoangson do you still encounter the problem? If yes, a simple change can work your problem around, executing runCancelable() (here and here) after releasing the lock. Something like:

    m.mu.Lock()
    // if we found a responder, call it
    if responder != nil {
        m.callCountInfo[key]++
        m.totalCallCount++
    } else {
        // we didn't find a responder, so fire the 'no responder' responder
        if m.noResponder == nil {
            m.mu.Unlock()
            return ConnectionFailure(req)
        }
        m.callCountInfo["NO_RESPONDER"]++
        m.totalCallCount++
        responder = m.noResponder
    }
    m.mu.Unlock()
    return runCancelable(responder, req)

[maxatome]

Done in #62

[daohoangson]

@maxatome it has been quite a while, we used the workaround above to pass the tests. Let me drop it and see whether it still works 🙏

[maxatome]

@daohoangson any news?

[maxatome]

62 merged.