Add Client method

[gmlewis]

Fixes: #1859.
Closes: #2015.

[codecov]

Codecov Report

Merging #2016 (6867ee3) into master (b8294c3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2016   +/-   ##
=======================================
  Coverage   97.86%   97.86%           
=======================================
  Files         107      107           
  Lines        6897     6901    +4     
=======================================
+ Hits         6750     6754    +4     
  Misses         81       81           
  Partials       66       66           

Impacted Files	Coverage Δ
github/github.go	97.10% <100.00%> (+0.03%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8294c3...6867ee3. Read the comment docs.

[gmlewis]

@bluekeyes and @jeffmendoza - what do you think of this solution? Feel free to LGTM+Approve if you agree.

[bluekeyes]

I might be missing something, but I'm not sure this solves the locking problem. When c modifies CheckRedirect in the future, it will hold the lock. But whoever called Client() won't have access to the lock and can modify the same CheckRedirect function without holding the lock.

I was thinking this would return a shallow copy:

Suggested change

	c.clientMu.Lock()
	defer c.clientMu.Unlock()
	return c.client
	c.clientMu.Lock()
	defer c.clientMu.Unlock()
	clientCopy := *c.client
	return &clientCopy

That way both c and the caller can modify CheckRedirect without conflict.

[gmlewis]

No, I don't think that is necessary. The point of the lock is to prevent two clients from attempting to overwrite the same variable at the same time (for handling redirects). A shallow copy is not necessary.

The point of solving #1859 is to use the same http.Client for older versions of this repo and it can be used concurrently without the need for a shallow copy.

[bluekeyes]

Hmm, I was thinking of a case like this (admittedly contrived):

import (
    "context"
    "github.com/google/go-github/v37/github"
    github36 "github.com/google/go-github/v36/github"
)

func useTwoVersionsConcurrently(ctx context.Context, client *github.Client) {
    client36 := github36.NewClient(client.Client())
    client36.BaseURL = client.BaseURL

    go func() {
        client.DownloadReleaseAsset(ctx, "octocat", "Hello-World", 1, http.DefaultClient)
    }()

    go func() {
        client36.MigrationArchiveURL(ctx, "ORG", 42)
    }()
}

I thought client.DownloadReleaseAsset and client36.MigrationArchiveURL would acquire different locks and then modify the CheckRedirect function on the same http.Client instance (the two GitHub clients share a pointer.) But I could easily be overlooking or misunderstanding something.

My understanding is that the Transport field is the most important piece to share between GitHub clients, as this has the authentication, caching, and other persistent state. The http.Client doesn't appear to have much state of its own.

[gmlewis]

@bluekeyes - I understand what you are saying, but completely disagree. What you wrote should work fine.

According to: https://pkg.go.dev/net/http

Clients and Transports are safe for concurrent use by multiple goroutines and for efficiency should only be created once and re-used.

Therefore, it is safe to use a single http.Client concurrently. That is not the purpose for the mutex and is not the problem being solved by it.

The purpose of the mutex is to prevent a "race condition" and is there to protect the actual variable Client.client itself. According to Wikipedia,

A race condition arises in software when a computer program, to operate properly, depends on the sequence or timing of the program's processes or threads. Critical race conditions cause invalid execution and software bugs. Critical race conditions often happen when the processes or threads depend on some shared state. Operations upon shared states are done in critical sections that must be mutually exclusive. Failure to obey this rule can corrupt the shared state.

Some of our methods temporarily swap out the value of Client.client in order to perform some cool features like following redirects to get the final values so that the users of this client library don't have to figure all that stuff out for themselves.

If the mutex were not there, it would be certainly possible for two concurrent goroutines to both attempt to swap out a new temporary client by both messing with the Client.client variable directly, and due to the race condition, you would have no way of knowing what value would be used or how it would end up. The mutex prevents this race condition.

Therefore, I stand by my assertion that this PR should be a valid solution to #1859.

[bluekeyes]

Thanks for taking the time to discuss this with me, @gmlewis. I hope I'm not being too difficult - I use this library a lot and really appreciate the time and effort you put into maintaining it, so I only want to help fix what I believe is a bug.

To that end, Go's race detector also indicates that the solution as currently implemented has a data race.

The purpose of the mutex is to prevent a "race condition" and is there to protect the actual variable Client.client itself.
[...]
Some of our methods temporarily swap out the value of Client.client in order to perform some cool features [...]

I think this is the source of our disagreement. Looking through the library, the two functions (1, 2) that hold client.clientMu update the Client.client.CheckRedirect field, not the Client.client field. If parts of the library do change the Client.client field, they seem to do so without holding the lock.

In other words, the current code uses clientMu to prevent concurrent modification of the CheckRedirect field. By returning a pointer to the Client.client, external code can now modify the same CheckRedirect field without holding the lock (which is private to the original Client.)

[gmlewis]

Wow, awesome, @bluekeyes !!! Looks like I'm wrong! I'm in a meeting... I'll take a look in a bit. Thank you!

[gmlewis]

I totally stand corrected... thank you for your persistence, @bluekeyes !!! I really appreciate it.

Have you tried running the race test with your shallow copy version? If it passes the race test, then I should be able to update this PR if you agree.

[bluekeyes]

Glad we figured things out! Using a library version that makes a shallow copy of the client passes the race detector test on my machine, so I think that will work fine (unless you or anyone else spots another issue.)

[gmlewis]

@bluekeyes - feel free to LGTM+Approve this and we can get it merged.

[bluekeyes]

LGTM!