h2non / gock

HTTP traffic mocking and testing made easy in Go ༼ʘ̚ل͜ʘ̚༽
https://pkg.go.dev/github.com/h2non/gock
MIT License
2.04k stars 106 forks source link

Set no global variables #114

Open wendorf opened 6 months ago

wendorf commented 6 months ago

gock sets http.DefaultTransport, which can cause data races with other code that may be reading it. gock uses a mutex when setting the transport, but packages that don't know about gock aren't using the mutex, so races are still possible.

For me, this shows up as unpredictable test failures depending on how goroutines and parallel tests are run. I believe the correct solution to this problem is to allow gock to operate in a way that does not write to any globals, especially those in packages not belonging to gock.

By creating a Gock struct and moving global variables to it instead, gock no longer mutates any globals, and multiple Gocks can be used simultaneously, allowing for parallel testing with gock.


This is probably not ready to merge, since it breaks the gock API. All package-level functions are removed and replaced with struct-level methods. I'm opening this PR to start a conversation about possible ways this could be modified to actually be merged, and to share this work if anyone else would benefit from it.

A couple possible directions to take to make this mergeable:

@h2non, I'd love your thoughts as to whether this is useful work for gock, or if it's better as something that just lives on as a fork. If it is useful, what changes would you like to see to make this mergeable?

h2non commented 6 months ago

Thanks for the work on this. Definitively this is useful and I'm inclined to merge this, but ideally current package-level interface should remain compatible as it is convenient and part of a better developer experience.

The global singleton instance can work, where both usage vectors can converge depending on the user requirements. If zero-breaking changes turn out to be too complicated, I will go with the v2 version bump.

Feel free to work on it and I will be happy to take a look once ready. I don't invest too much time on OSS these days so it may take a while for me to reply, so no rush at all!

wendorf commented 6 months ago

That all makes sense! I've moved the new implementation to a threadsafe subpackage and reimplemented the root package using it, to avoid duplicate code. In order to do so, there had to be a couple of breaking changes that I think are low-impact but certainly not zero.

From the commit description:

I tried to make this as non-breaking of a change as possible, but it could not be done without some breaking changes:

  • Exported types are just exposing threadsafe types. For example, type MatchFunc is changed from type MatchFunc func(*http.Request, *Request) (bool, error) to type MatchFunc = threadsafe.MatchFunc. The ergonomics of using these types should be unchanged, but it is technically breaking.
  • Some package-level variables were exposed to allow dynamic configuration, like MatchersHeader. To correctly use the *threadsafe.Gock instance, I had to replace the var with a getter function and add a setter function. For getter use cases, users will just have to append () to call the function, but for setter use cases they will need to modify their code a little more (especially if they were doing something like appending to the slice).

Other notable things:

  • I tried to leave as much of the original test suite as possible to prove that this refactor is correct. That means there are some unnecessarily duplicated tests between the root package and threadsafe, so there's an opportunity for cleanup.
  • Some root-level tests relied on unexported symbols which are no longer available to those tests. Some were able to be updated using exported getters, but some were deleted. I believe the deleted tests were not providing additional value because of the above-mentioned duplication.
  • To correctly maintain the getting and setting of http.DefaultTransport, I added "callback" methods for threadsafe.Gock: DisableCallback, InterceptCallback, and InterceptingCallback. The root package sets these on the `var g threadsafe.Gockvariable, and the functions are responsible for reading or writing http.DefaultTransport. Implementing this logic in the original functions (e.g.gock.Disable`) proved too odd since the some of the functions call others. We would have to retain some duplicate implementation logic to run the logic in the right place, so the callback methods felt like the cleanest workaround.

I've also tested that all the _examples compile and the tests pass (though _examples/persistent/TestPersistent was previously failing and continues to).

If you'd prefer any other changes to this, like leaving the original implementation untouched so this isn't a breaking change or renaming anything like the threadsafe package, let me know and I'm happy to do so.