Skip to content

Put sync.Mutex on top of variables it protects. #396

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

Closed
wants to merge 1 commit into from
Closed

Put sync.Mutex on top of variables it protects. #396

wants to merge 1 commit into from

Conversation

dmitshur
Copy link
Member

@dmitshur dmitshur commented Jul 9, 2016

This change makes the code more consistent with the rateMu below.

This is a followup to #390 (comment).

I've also considered an alternative solution that kept the comments on top

// clientMu protects the client during calls that modify the CheckRedirect func.
clientMu sync.Mutex
// HTTP client used to communicate with the API.
client *http.Client

or

clientMu sync.Mutex // clientMu protects the client during calls that modify the CheckRedirect func.
// HTTP client used to communicate with the API.
client *http.Client

but decided to go with the inline version because I figured these are internal implementation details and unexported variables, so it's fine.

If there's any preference towards keeping the comments on top, I'm happy to change it back.

This change makes the code more consistent with the rateMu below.

This is a followup to #390 (comment).
@gmlewis
Copy link
Collaborator

gmlewis commented Jul 10, 2016

LGTM 👍
Awaiting second LGTM before merging.

@gmlewis gmlewis closed this in 136ef5c Jul 13, 2016
@gmlewis
Copy link
Collaborator

gmlewis commented Jul 13, 2016

Incorporated in 136ef5c

@dmitshur dmitshur deleted the use-mutex-hat-for-clientMu branch July 13, 2016 17:43
bubg-dev pushed a commit to bubg-dev/go-github that referenced this pull request Jun 16, 2017
This change makes the code more consistent with the rateMu below.

This is a followup to google#390 (comment).

Closes google#396.

Change-Id: I0fc8b280911f9171fb4d8e3fa60cf6d53b64a65b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants