Add InitWithClient and deprecate InitClient because we don't want to copy Client's field sync.RWMutex by value #273
Conversation
…led sync.RWMutex by value
update the local repo
|
I think this may not be allowed actually since it changes the API surface... |
|
hmm, I can also add a new API. |
kurtisvg
added
Status: Review Needed
type: feature request
|
Can you tell us about the motivation behind this change? Is there a bug? |
|
Copying a sync.RWMutex is not generally safe, although I think copying an unused (i.e. zero-value) RWMutex can't be noticed by the internal code used to detect this copying. In any case this was definitely an oversight when I wrote this API and the API should be either changed or deprecated/deleted-eventually. |
|
@broady and I discussed this today and the conclusion was that we should defiantly make this change, but play it safe and create a new function and deprecate the old one. |
|
@kurtisvg Thanks! Is it part of your plan? Or I will do it later. :-) |
|
@mandyh2018 If you would like to make the change you are welcome to, otherwise I will update this PR sometime before the next release. |
|
broady@ are we okay with changing the existing API? I thought we want to add a new one instead. |
|
kurtisvg@ Are you working on this? If not, I will add a new API this week. :-) |
|
My plan is to just add a new function (likely |
|
Let me work on this then. I have some time today and tomorrow. :-) So I just close this pull request and send a new one? kurtisvg@ |
|
You can just update this PR with the changes |
… copying sync.RWMutex is not safe.
|
I updated this PR, please let me know if any comment. Thanks! |
mandyh2018
changed the title
mandyh2018
changed the title
proxy/proxy/dial.go
Outdated
| @@ -80,12 +81,22 @@ func Init(auth *http.Client, connset *ConnSet, dialer Dialer) { | |||
|
|
|||
| // InitClient is similar to Init, but allows you to specify the Client | |||
| // directly. | |||
|
|
|||
| // Deprecated: Client contains sync.RWMutex, which should not be copied by value. | |||
There was a problem hiding this comment.
just "Deprecated: Use InitWithClient instead." i think.
Sync with the latest change# Please enter a commit message to explain why this merge is necessary,
|
Thank you! I will wait until I become the authorized user? |
|
I can see the warning: |
|
Thanks! Merged. |
|
Thank you! |
Init proxy client using pointer because we don't want to copy its filed sync.RWMutex by value.