Skip to content

Add Client method #2016

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jul 21, 2021
Merged

Add Client method #2016

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e708608
Add Client method
gmlewis Jul 20, 2021
9a0e1df
Add test for coverage
gmlewis Jul 20, 2021
6867ee3
Address review feedback to eliminate race condition
gmlewis Jul 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions github/github.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,13 @@ type service struct {
client *Client
}

// Client returns the http.Client used by this GitHub client.
func (c *Client) Client() *http.Client {
c.clientMu.Lock()
defer c.clientMu.Unlock()
return c.client
Copy link
Contributor

@bluekeyes bluekeyes Jul 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@gmlewis gmlewis Jul 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@bluekeyes bluekeyes Jul 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@gmlewis gmlewis Jul 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@bluekeyes bluekeyes Jul 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Collaborator Author

@gmlewis gmlewis Jul 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@gmlewis gmlewis Jul 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@bluekeyes bluekeyes Jul 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

}

// ListOptions specifies the optional parameters to various List methods that
// support offset pagination.
type ListOptions struct {
Expand Down
8 changes: 8 additions & 0 deletions github/github_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,14 @@ func TestNewClient(t *testing.T) {
}
}

func TestClient(t *testing.T) {
c := NewClient(nil)
c2 := c.Client()
if c.client != c2 {
t.Error("Client returned different http.Client, but should be the same.")
}
}

func TestNewEnterpriseClient(t *testing.T) {
baseURL := "https://custom-url/api/v3/"
uploadURL := "https://custom-upload-url/api/uploads/"
Expand Down