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

Issue 9929043: code review 9929043: go.crypto/ssh: fix race on mock ssh network connection (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 7 months ago by dfc
Modified:
8 years, 7 months ago
Reviewers:
agl1
CC:
golang-dev, albert.strasheim, huin-google, kardia
Visibility:
Public.

Description

go.crypto/ssh: fix race on mock ssh network connection Fixes issue 5138. Fixes issue 4703. This appears to pass my stress tests with and without the -race detector, but I'd like to see others hit it with their machines.

Patch Set 1 #

Patch Set 2 : diff -r 273987d8ccbc https://code.google.com/p/go.crypto #

Patch Set 3 : diff -r 273987d8ccbc https://code.google.com/p/go.crypto #

Patch Set 4 : diff -r 7248f59503a8 https://code.google.com/p/go.crypto #

Patch Set 5 : diff -r 7248f59503a8 https://code.google.com/p/go.crypto #

Patch Set 6 : diff -r 7248f59503a8 https://code.google.com/p/go.crypto #

Total comments: 2

Patch Set 7 : diff -r 7248f59503a8 https://code.google.com/p/go.crypto #

Total comments: 9

Patch Set 8 : diff -r 7248f59503a8 https://code.google.com/p/go.crypto #

Patch Set 9 : diff -r 7248f59503a8 https://code.google.com/p/go.crypto #

Unified diffs Side-by-side diffs Delta from patch set Stats (+69 lines, -10 lines) Patch
M ssh/test/test_unix_test.go View 1 2 3 4 5 6 7 3 chunks +69 lines, -10 lines 0 comments Download

Messages

Total messages: 14
dfc
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go.crypto
8 years, 7 months ago (2013-06-01 00:54:23 UTC) #1
dfc
Hello golang-dev@googlegroups.com, fullung@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
8 years, 7 months ago (2013-06-01 01:25:51 UTC) #2
albert.strasheim
Looks pretty good. I still need to take a closer look at the mock conn. ...
8 years, 7 months ago (2013-06-01 06:56:57 UTC) #3
dfc
> s.cmd.Wait() That was incorporated into s.Shutdown by dsymonds a while back. Please let me ...
8 years, 7 months ago (2013-06-01 06:59:43 UTC) #4
dfc
On 2013/06/01 06:59:43, dfc wrote: > > s.cmd.Wait() > > That was incorporated into s.Shutdown ...
8 years, 7 months ago (2013-06-02 01:45:52 UTC) #5
huin-google
Just the one question, although I'm not familiar with this section of the code, so ...
8 years, 7 months ago (2013-06-03 07:59:56 UTC) #6
albert.strasheim
Hello On 2013/06/02 01:45:52, dfc wrote: > On 2013/06/01 06:59:43, dfc wrote: > > > ...
8 years, 7 months ago (2013-06-03 08:09:26 UTC) #7
dfc
> This still seems to leak processes. > > If you run with -test.cpu=1,1,1,1,1,1,1,... > ...
8 years, 7 months ago (2013-06-03 08:43:14 UTC) #8
dfc
https://codereview.appspot.com/9929043/diff/13001/ssh/test/test_unix_test.go File ssh/test/test_unix_test.go (left): https://codereview.appspot.com/9929043/diff/13001/ssh/test/test_unix_test.go#oldcode157 ssh/test/test_unix_test.go:157: // newServer returns a new mock ssh server. On ...
8 years, 7 months ago (2013-06-03 08:43:42 UTC) #9
dfc
Hello golang-dev@googlegroups.com, fullung@gmail.com, huin@google.com, kardianos@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
8 years, 7 months ago (2013-06-03 08:43:42 UTC) #10
agl1
LGTM https://codereview.appspot.com/9929043/diff/23001/ssh/test/test_unix_test.go File ssh/test/test_unix_test.go (right): https://codereview.appspot.com/9929043/diff/23001/ssh/test/test_unix_test.go#newcode154 ssh/test/test_unix_test.go:154: sysref int Comment for sysref? What it does ...
8 years, 7 months ago (2013-06-04 16:53:53 UTC) #11
dfc
Thank you for your review. I've made the changes you requested and added some more ...
8 years, 7 months ago (2013-06-05 01:18:59 UTC) #12
dfc
*** Submitted as https://code.google.com/p/go/source/detail?r=553b87316697&repo=crypto *** go.crypto/ssh: fix race on mock ssh network connection Fixes issue ...
8 years, 7 months ago (2013-06-05 01:19:34 UTC) #13
agl1
8 years, 7 months ago (2013-06-05 01:46:08 UTC) #14
Message was sent while issue was closed.
https://codereview.appspot.com/9929043/diff/23001/ssh/test/test_unix_test.go
File ssh/test/test_unix_test.go (right):

https://codereview.appspot.com/9929043/diff/23001/ssh/test/test_unix_test.go#...
ssh/test/test_unix_test.go:173: if closing {
On 2013/06/05 01:19:00, dfc wrote:
> On 2013/06/04 16:53:53, agl1 wrote:
> > could be just c.closing = closing.
> 
> This check is needed so that c.closing does not fall back to false.

Unimportant, but we know that c.closing is false because, if it were true, then
we would return on line 170.
Sign in to reply to this message.

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