Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(10)

Issue 12727043: code review 12727043: go.net: add netutil package with LimitListener (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 10 months ago by adg
Modified:
8 years, 10 months ago
Reviewers:
ngaut1, rsc, rog
CC:
golang-dev, dsymonds, rsc
Visibility:
Public.

Description

go.net: add netutil package with LimitListener Update issue 6012

Patch Set 1 #

Patch Set 2 : diff -r 784c29549db7 https://code.google.com/p/go.net #

Patch Set 3 : diff -r cfb4d23ab57e https://code.google.com/p/go.net #

Total comments: 6

Patch Set 4 : diff -r cfb4d23ab57e https://code.google.com/p/go.net #

Total comments: 1

Patch Set 5 : diff -r cfb4d23ab57e https://code.google.com/p/go.net #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+109 lines, -0 lines) Patch
A netutil/listen.go View 1 2 3 1 chunk +44 lines, -0 lines 5 comments Download
A netutil/listen_test.go View 1 2 3 1 chunk +65 lines, -0 lines 0 comments Download

Messages

Total messages: 10
adg
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go.net
8 years, 10 months ago (2013-08-12 00:16:33 UTC) #1
dsymonds
cute. I don't have an opinion on whether this should be in go.net, but here's ...
8 years, 10 months ago (2013-08-12 00:36:00 UTC) #2
adg
https://codereview.appspot.com/12727043/diff/6001/netutil/listen.go File netutil/listen.go (right): https://codereview.appspot.com/12727043/diff/6001/netutil/listen.go#newcode37 netutil/listen.go:37: ch chan struct{} On 2013/08/12 00:36:00, dsymonds wrote: > ...
8 years, 10 months ago (2013-08-12 00:44:32 UTC) #3
dsymonds
https://codereview.appspot.com/12727043/diff/6001/netutil/listen_test.go File netutil/listen_test.go (right): https://codereview.appspot.com/12727043/diff/6001/netutil/listen_test.go#newcode32 netutil/listen_test.go:32: go http.Serve(l, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { On 2013/08/12 ...
8 years, 10 months ago (2013-08-12 00:50:42 UTC) #4
adg
On 12 August 2013 10:50, <dsymonds@golang.org> wrote: > httptest.Server exposes the Listener for exactly this ...
8 years, 10 months ago (2013-08-12 00:52:12 UTC) #5
rsc
LGTM https://codereview.appspot.com/12727043/diff/7003/netutil/listen.go File netutil/listen.go (right): https://codereview.appspot.com/12727043/diff/7003/netutil/listen.go#newcode25 netutil/listen.go:25: add a Close method on limitListener that will ...
8 years, 10 months ago (2013-08-13 17:40:25 UTC) #6
adg
*** Submitted as https://code.google.com/p/go/source/detail?r=ac736dc34602&repo=net *** go.net: add netutil package with LimitListener Update issue 6012 R=golang-dev, ...
8 years, 10 months ago (2013-08-14 01:00:11 UTC) #7
ngaut1
i think we should check if error is nil, programmers may use two goroutines to ...
8 years, 10 months ago (2013-08-14 02:28:54 UTC) #8
rog
I think ngautl has a good point about the Close logic - it's not uncommon ...
8 years, 10 months ago (2013-08-14 08:02:09 UTC) #9
adg
8 years, 10 months ago (2013-08-15 01:19:57 UTC) #10
Message was sent while issue was closed.
https://codereview.appspot.com/12727043/diff/20001/netutil/listen.go
File netutil/listen.go (right):

https://codereview.appspot.com/12727043/diff/20001/netutil/listen.go#newcode15
netutil/listen.go:15: for i := 0; i < n; i++ {
On 2013/08/14 08:02:09, rog wrote:
> I don't think you actually need to fill
> the channel here - we're not using the
> channel as a memory barrier here -
> I believe it'll work just as effectively
> for this purpose if you start empty and
> fill it up.
> 
> This may make a difference for if the maximum
> number of connections is quite high.

The allocation of the buffer is the expensive part. Filling it up should be a
secondary concern.

To me it is clearer to put "connection tokens" in the buffer and to "give" one
to each active connection.

Let's worry about efficiency if it actually affects someone.

https://codereview.appspot.com/12727043/diff/20001/netutil/listen.go#newcode42
netutil/listen.go:42: l.ch <- struct{}{}
Thanks for the suggestion guys.

I went with sync.Once: https://codereview.appspot.com/12967043/
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b