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

Set no global variables #114

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

wendorf
Copy link

@wendorf wendorf commented Dec 25, 2023

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:

  • Introduce the changes as a gock/v2 package
  • Reimplement the package-level methods as accessors of a singleton instance of the struct, and re-introducing the writes to http.DefaultTransport

@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?

@wendorf wendorf marked this pull request as ready for review December 25, 2023 17:28
@h2non
Copy link
Owner

h2non commented Dec 28, 2023

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!

@h2non h2non assigned h2non and wendorf and unassigned h2non Dec 28, 2023
@h2non h2non self-requested a review December 28, 2023 19:02
The root gock package is not threadsafe because it modifies
http.DefaultTransport. This can cause a race condition when other code
is reading http.DefaultTransport.

`threadsafe` is a reimplementation of the entire root package using
a new `gock` struct to hold data that was previously global.
To avoid having and maintaining duplicate code, the root not-threadsafe
package is reimplemented using a globl instance of *threadsafe.Gock.

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.Gock` variable, 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.
@wendorf
Copy link
Author

wendorf commented Jan 8, 2024

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.Gock variable, 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.

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

Successfully merging this pull request may close these issues.

2 participants