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

Issue 6719050: code review 6719050: go.talks/present: restructure socket.go to avoid races (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 5 months ago by adg
Modified:
11 years, 5 months ago
Reviewers:
CC:
dvyukov, r, golang-dev
Visibility:
Public.

Description

go.talks/present: restructure socket.go to avoid races

Patch Set 1 #

Total comments: 4

Patch Set 2 : diff -r 764d8c2ea85e https://code.google.com/p/go.talks #

Total comments: 5

Patch Set 3 : diff -r 2147a6840a68 https://code.google.com/p/go.talks #

Total comments: 4

Patch Set 4 : diff -r 2147a6840a68 https://code.google.com/p/go.talks #

Patch Set 5 : diff -r 2147a6840a68 https://code.google.com/p/go.talks #

Patch Set 6 : diff -r 2147a6840a68 https://code.google.com/p/go.talks #

Patch Set 7 : diff -r 2147a6840a68 https://code.google.com/p/go.talks #

Unified diffs Side-by-side diffs Delta from patch set Stats (+117 lines, -143 lines) Patch
M present/socket.go View 1 2 3 4 5 5 chunks +117 lines, -143 lines 0 comments Download

Messages

Total messages: 9
adg
Hello dvyukov, r (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go.talks
11 years, 5 months ago (2012-10-16 22:03:07 UTC) #1
r
http://codereview.appspot.com/6719050/diff/1/present/socket.go File present/socket.go (right): http://codereview.appspot.com/6719050/diff/1/present/socket.go#newcode167 present/socket.go:167: // end sends the an end message to the ...
11 years, 5 months ago (2012-10-16 22:07:54 UTC) #2
adg
The principal change here is that Process.run is now split into Run and Wait. Run ...
11 years, 5 months ago (2012-10-16 22:16:16 UTC) #3
r
LGTM but wait for or at least TBR= dvyukov
11 years, 5 months ago (2012-10-17 03:00:46 UTC) #4
dvyukov
https://codereview.appspot.com/6719050/diff/6001/present/socket.go File present/socket.go (right): https://codereview.appspot.com/6719050/diff/6001/present/socket.go#newcode135 present/socket.go:135: c.Lock() Do we need this mutex? https://codereview.appspot.com/6719050/diff/6001/present/socket.go#newcode146 present/socket.go:146: c.end(id, ...
11 years, 5 months ago (2012-10-17 03:33:05 UTC) #5
adg
PTAL https://codereview.appspot.com/6719050/diff/6001/present/socket.go File present/socket.go (right): https://codereview.appspot.com/6719050/diff/6001/present/socket.go#newcode135 present/socket.go:135: c.Lock() On 2012/10/17 03:33:05, dvyukov wrote: > Do ...
11 years, 5 months ago (2012-10-17 04:12:08 UTC) #6
r
LGTM again https://codereview.appspot.com/6719050/diff/8001/present/socket.go File present/socket.go (right): https://codereview.appspot.com/6719050/diff/8001/present/socket.go#newcode141 present/socket.go:141: done chan struct{} // closed when run ...
11 years, 5 months ago (2012-10-17 04:22:09 UTC) #7
adg
https://codereview.appspot.com/6719050/diff/8001/present/socket.go File present/socket.go (right): https://codereview.appspot.com/6719050/diff/8001/present/socket.go#newcode141 present/socket.go:141: done chan struct{} // closed when run complete On ...
11 years, 5 months ago (2012-10-17 04:39:12 UTC) #8
adg
11 years, 5 months ago (2012-10-17 04:51:39 UTC) #9
*** Submitted as
http://code.google.com/p/go/source/detail?r=965aebfbb749&repo=talks ***

go.talks/present: restructure socket.go to avoid races

R=dvyukov, r
CC=golang-dev
http://codereview.appspot.com/6719050
Sign in to reply to this message.

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