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.

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

Jump to bottom

net/http, net/http/httptest: locks being copied #16228

Closed
josharian opened this issue Jun 30, 2016 · 7 comments
Closed

net/http, net/http/httptest: locks being copied #16228

josharian opened this issue Jun 30, 2016 · 7 comments
Labels
FrozenDueToAge
Milestone
Go1.8Early

Comments

@josharian
Copy link
Contributor

From cmd/vet:

net/http/h2_bundle.go:5133: assignment copies lock value to *cfg: crypto/tls.Config contains sync.Once contains sync.Mutex
net/http/httptest/server.go:119: assignment copies lock value to *s.TLS: crypto/tls.Config contains sync.Once contains sync.Mutex

These seem like legitimate complaints.

It seems like tls.Config could use a Copy method, but probably not for Go 1.7.

cc @bradfitz @ianlancetaylor
@josharian josharian added this to the Go1.7Maybe milestone Jun 30, 2016
@odeke-em
Copy link
Member

Related to #15177?

@josharian
Copy link
Contributor Author

Yes, probably. It's not obvious to me from the immediate context why these are ok, though.

If they are ok, they're yet more (new?) false positives...in which case, #16227 is related.

cc @valyala

@valyala
Copy link
Contributor

valyala commented Jul 1, 2016 •
edited
Loading

Both cases look buggy, since they overwrite already existing mutex inside tls.Config with a new one.

@adg adg modified the milestones: Go1.8Early, Go1.7Maybe Jul 6, 2016
@josharian josharian mentioned this issue Aug 29, 2016
Closed
@bradfitz bradfitz self-assigned this Aug 30, 2016
@gopherbot
Copy link
Contributor

CL https://golang.org/cl/28075 mentions this issue.

gopherbot pushed a commit that referenced this issue Sep 1, 2016
@bradfitz
crypto/tls: add Config.Clone
d24f446 
In Go 1.0, the Config struct consisted only of exported fields.

In Go 1.1, it started to grow private, uncopyable fields (sync.Once,
sync.Mutex, etc).

Ever since, people have been writing their own private Config.Clone
methods, or risking it and doing a language-level shallow copy and
copying the unexported sync variables.

Clean this up and export the Config.clone method as Config.Clone.
This matches the convention of Template.Clone from text/template and
html/template at least.

Fixes #15771
Updates #16228 (needs update in x/net/http2 before fixed)
Updates #16492 (not sure whether @agl wants to do more)

Change-Id: I48c2825d4fef55a75d2f99640a7079c56fce39ca
Reviewed-on: https://go-review.googlesource.com/28075
Run-TryBot: Brad Fitzpatrick <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Andrew Gerrand <[email protected]>
@bradfitz
Copy link
Contributor

bradfitz commented Sep 1, 2016

d24f446 did the first half, and https://go-review.googlesource.com/28344 is the http2 part, which will be bundled in to std before Go 1.8. Closing.

@bradfitz bradfitz closed this as completed Sep 1, 2016
@gopherbot
Copy link
Contributor

CL https://golang.org/cl/28344 mentions this issue.

gopherbot pushed a commit to golang/net that referenced this issue Sep 1, 2016
@bradfitz
http2: fix all vet warnings
1358eff 
Updates golang/go#16228
Updates golang/go#11041

Change-Id: I2b50c2f4bfaae2d9ad59bc78e1c7c3e807f08075
Reviewed-on: https://go-review.googlesource.com/28344
Run-TryBot: Brad Fitzpatrick <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
@gopherbot
Copy link
Contributor

CL https://golang.org/cl/29110 mentions this issue.

gopherbot pushed a commit that referenced this issue Sep 13, 2016
@bradfitz
net/http: update bundled x/net/http2
802cb59 
Updates x/net/http2 (and x/net/lex/httplex) to git rev 749a502 for:

   http2: don't sniff first Request.Body byte in Transport until we have a conn
   https://golang.org/cl/29074
   Fixes #17071

   http2: add Transport support for unicode domain names
   https://golang.org/cl/29071
   Updates #13835

   http2: don't send bogus :path pseudo headers if Request.URL.Opaque is set
   https://golang.org/cl/27632
     +
   http2: fix bug where '*' as a valid :path value in Transport
   https://golang.org/cl/29070
   Updates #16847

   http2: fix all vet warnings
   https://golang.org/cl/28344
   Updates #16228
   Updates #11041

Also uses the new -underscore flag to x/tools/cmd/bundle from
https://golang.org/cl/29086

Change-Id: Ica0f6bf6e33266237e37527a166a783d78c059c4
Reviewed-on: https://go-review.googlesource.com/29110
Run-TryBot: Brad Fitzpatrick <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Chris Broadfoot <[email protected]>
@golang golang locked and limited conversation to collaborators Sep 13, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.8Early
Development

No branches or pull requests
6 participants
@bradfitz @josharian @valyala @odeke-em @adg @gopherbot