[net] http2: make the error channel pool per-Server
Damien Neil (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Brad Fitzpatrick
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brad Fitzpatrick (Gerrit)
Brad Fitzpatrick voted and added 2 comments![]( "Open in Gerrit")
Votes added by Brad Fitzpatrick
| Code-Review | +2 |
2 comments
return make(chan error, 1) // Server used without calling ConfigureServermaybe revert to the package global variable form like before in this case? otherwise if this is a valid way to use a Server, it could be a surprising performance difference
errChanPool: sync.Pool{New: func() interface{} { return make(chan error, 1) }},do we not use "any" in x/ repos yet?
Related details
- Brad Fitzpatrick
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brad Fitzpatrick (Gerrit)
Brad Fitzpatrick added 1 comment![]( "Open in Gerrit")
return make(chan error, 1) // Server used without calling ConfigureServermaybe revert to the package global variable form like before in this case? otherwise if this is a valid way to use a Server, it could be a surprising performance difference
Or can we put a sync.Once on serverInternalState and add an errChanPool accessor method that does the lazy init, removing it from ConfigureServer?
Damien Neil (Gerrit)
Damien Neil added 2 comments![]( "Open in Gerrit")
return make(chan error, 1) // Server used without calling ConfigureServerBrad Fitzpatrickmaybe revert to the package global variable form like before in this case? otherwise if this is a valid way to use a Server, it could be a surprising performance difference
Or can we put a sync.Once on serverInternalState and add an errChanPool accessor method that does the lazy init, removing it from ConfigureServer?
Good thought on reverting to package global. Did that.
The `s == nil` case occurs when `Server.state` is uninitialized (nil). We'd need a way to lazily set `Server.state`, which is tricky: We could make it atomic (probably no real performance cost, since it'd be mostly uncontended?), or we could make an unsynchronized initialization in `Server.serveConn` (probably fine, but could cause race conditions for some people).
I'd rather not figure any of that out right now.
errChanPool: sync.Pool{New: func() interface{} { return make(chan error, 1) }},do we not use "any" in x/ repos yet?
We do, but we haven't done a cleanup pass to change existing `interface {}`s.
Related details
- Brad Fitzpatrick
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Palmer (Gerrit)
Brian Palmer added 1 comment![]( "Open in Gerrit")
return make(chan error, 1) // Server used without calling ConfigureServerBrad Fitzpatrickmaybe revert to the package global variable form like before in this case? otherwise if this is a valid way to use a Server, it could be a surprising performance difference
Damien NeilOr can we put a sync.Once on serverInternalState and add an errChanPool accessor method that does the lazy init, removing it from ConfigureServer?
Good thought on reverting to package global. Did that.
The `s == nil` case occurs when `Server.state` is uninitialized (nil). We'd need a way to lazily set `Server.state`, which is tricky: We could make it atomic (probably no real performance cost, since it'd be mostly uncontended?), or we could make an unsynchronized initialization in `Server.serveConn` (probably fine, but could cause race conditions for some people).
I'd rather not figure any of that out right now.
Falling back to the global pool does mean that when `Server.state` is uninitialized, tests will sporadically see the same failures that we are seeing in golang/go#75674 right? I'm not clear on how common that state is.
Related details
- Brad Fitzpatrick
- Damien Neil
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Neil (Gerrit)
Damien Neil added 1 comment![]( "Open in Gerrit")
return make(chan error, 1) // Server used without calling ConfigureServerBrad Fitzpatrickmaybe revert to the package global variable form like before in this case? otherwise if this is a valid way to use a Server, it could be a surprising performance difference
Damien NeilOr can we put a sync.Once on serverInternalState and add an errChanPool accessor method that does the lazy init, removing it from ConfigureServer?
Brian PalmerGood thought on reverting to package global. Did that.
The `s == nil` case occurs when `Server.state` is uninitialized (nil). We'd need a way to lazily set `Server.state`, which is tricky: We could make it atomic (probably no real performance cost, since it'd be mostly uncontended?), or we could make an unsynchronized initialization in `Server.serveConn` (probably fine, but could cause race conditions for some people).
I'd rather not figure any of that out right now.
Falling back to the global pool does mean that when `Server.state` is uninitialized, tests will sporadically see the same failures that we are seeing in golang/go#75674 right? I'm not clear on how common that state is.
The global pool fallback should (I think) happen only for programs serving connections directly with `http2.Server.ServeConn`. I don't think that's very common. The much more common case of using `http.Server.ListenAndServeTLS` (or similar) will initialize the HTTP/2 server state and use a per-server pool.
Related details
- Brad Fitzpatrick
- Brian Palmer
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Neil (Gerrit)
Damien Neil uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Brad Fitzpatrick
- Brian Palmer
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin voted![]( "Open in Gerrit")
| Auto-Submit | +1 |
| Code-Review | +2 |
Related details
- Brad Fitzpatrick
- Brian Palmer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin voted Code-Review+1![]( "Open in Gerrit")
- Brad Fitzpatrick
- Brian Palmer
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
Gopher Robot submitted the change![]( "Open in Gerrit")
Change information
http2: make the error channel pool per-Server
Channels can't be shared across synctest bubbles, so a global pool causes
panics when using an http2.Server in a bubble. Make the pool per-Server.
A Server can't be shared across bubbles anyway (it contains channels)
and outside of tests most programs will have a single Server.
Fixes golang/go#75674
- M http2/http2.go
- M http2/http2_test.go
- M http2/server.go
Code-Review: +2 by Nicholas Husin, +2 by Brad Fitzpatrick, +1 by Nicholas Husin
TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |