Code review - Issue 14532048: code review 14532048: go.crypto/ssh: implement memTransport using sync.Cond.https://codereview.appspot.com/2013-10-29T16:13:22+00:00rietveld
Message from unknown
2013-10-09T12:50:41+00:00hanwen-googleurn:md5:cb25c000d6402f799583b2667f57d923
Message from unknown
2013-10-09T12:50:44+00:00hanwen-googleurn:md5:bddc070bfe2044f8e563989f1d4e0f52
Message from unknown
2013-10-09T12:51:03+00:00hanwen-googleurn:md5:552e05d62a62073961b4ca30afd37fcc
Message from hanwen@google.com
2013-10-09T12:51:08+00:00hanwen-googleurn:md5:65abe182b55513eb7201e0fd6c12fe27
Hello golang-dev@googlegroups.com (cc: agl@golang.org, dave@cheney.net),
I'd like you to review this change to
https://code.google.com/p/go.crypto
Message from hanwen@google.com
2013-10-09T12:51:57+00:00hanwen-googleurn:md5:8a259bfa2257ce10c13cfe183ab8e8b3
by request of dfc, this is split off from 14225043.
Message from dave@cheney.net
2013-10-11T06:16:51+00:00dfcurn:md5:396fcfaf223489dd10cd8b4eadbe8e4c
Sorry for the delay. I appreciate you splitting this change up, i'll try to turn around subsequent reviews faster.
https://codereview.appspot.com/14532048/diff/6001/ssh/mempipe_test.go
File ssh/mempipe_test.go (right):
https://codereview.appspot.com/14532048/diff/6001/ssh/mempipe_test.go#newcode89
ssh/mempipe_test.go:89: func TestmemPipeSafety(t *testing.T) {
I think this test is testing that a Close doesn't block a writePacket. Shouldn't it have some kind of select with a timeout clause that fires if writePacket doesn't complete in time ?
https://codereview.appspot.com/14532048/diff/6001/ssh/mempipe_test.go#newcode97
ssh/mempipe_test.go:97: if err := a.Close(); err != nil {
mempipe.Close never returns an error, what is this testing ?
Message from hanwen@google.com
2013-10-11T07:46:52+00:00hanwen-googleurn:md5:dff946834c04b34dc7cc7be412490dc3
https://codereview.appspot.com/14532048/diff/6001/ssh/mempipe_test.go
File ssh/mempipe_test.go (right):
https://codereview.appspot.com/14532048/diff/6001/ssh/mempipe_test.go#newcode97
ssh/mempipe_test.go:97: if err := a.Close(); err != nil {
On 2013/10/11 06:16:51, dfc wrote:
> mempipe.Close never returns an error, what is this testing ?
it's just to make sure the race detector doesn't see badness here.
Message from unknown
2013-10-11T12:26:44+00:00hanwen-googleurn:md5:791cc15a4fe34c4be3018c049439cbb6
Message from dave@cheney.net
2013-10-11T13:03:45+00:00dfcurn:md5:ee132606d81aa2f44bf6565d5e05bcdd
https://codereview.appspot.com/14532048/diff/6001/ssh/mempipe_test.go
File ssh/mempipe_test.go (right):
https://codereview.appspot.com/14532048/diff/6001/ssh/mempipe_test.go#newcode97
ssh/mempipe_test.go:97: if err := a.Close(); err != nil {
On 2013/10/11 07:46:52, hanwen-google wrote:
> On 2013/10/11 06:16:51, dfc wrote:
> > mempipe.Close never returns an error, what is this testing ?
>
> it's just to make sure the race detector doesn't see badness here.
why can't it just be
go func() {
a.Close()
}()
?
Message from unknown
2013-10-11T14:39:03+00:00hanwen-googleurn:md5:e12935bd8d3186a8da1ab5ea4fa5f924
Message from hanwen@google.com
2013-10-11T14:39:43+00:00hanwen-googleurn:md5:debefcf49ef8c60227c07162991a9df2
I've made double close return error too.
https://codereview.appspot.com/14532048/diff/6001/ssh/mempipe_test.go
File ssh/mempipe_test.go (right):
https://codereview.appspot.com/14532048/diff/6001/ssh/mempipe_test.go#newcode89
ssh/mempipe_test.go:89: func TestmemPipeSafety(t *testing.T) {
On 2013/10/11 06:16:51, dfc wrote:
> I think this test is testing that a Close doesn't block a writePacket. Shouldn't
> it have some kind of select with a timeout clause that fires if writePacket
> doesn't complete in time ?
If it blocks, the deadlock detector will make things crash.
https://codereview.appspot.com/14532048/diff/6001/ssh/mempipe_test.go#newcode97
ssh/mempipe_test.go:97: if err := a.Close(); err != nil {
On 2013/10/11 07:46:52, hanwen-google wrote:
> On 2013/10/11 06:16:51, dfc wrote:
> > mempipe.Close never returns an error, what is this testing ?
>
> it's just to make sure the race detector doesn't see badness here.
(added comment.)
Message from unknown
2013-10-11T14:41:38+00:00hanwen-googleurn:md5:80a3527fbe44fd8d8a295a2a9b123052
Message from dave@cheney.net
2013-10-12T10:57:03+00:00dfcurn:md5:5d6790dbf6e702146eb2cf7cf82e8554
https://codereview.appspot.com/14532048/diff/6001/ssh/mempipe_test.go
File ssh/mempipe_test.go (right):
https://codereview.appspot.com/14532048/diff/6001/ssh/mempipe_test.go#newcode89
ssh/mempipe_test.go:89: func TestmemPipeSafety(t *testing.T) {
I'd like to see this CL committed so we can move on to the real change.
Please remove this test and propose it as a separate CL so we can debate it without blocking the rest of the CL.
Message from unknown
2013-10-12T12:40:53+00:00hanwen-googleurn:md5:6da19ed8230a9ab9a48b9aada0f1c9fe
Message from hanwen@google.com
2013-10-12T12:48:31+00:00hanwen-googleurn:md5:f133e635c98ec7b1985ca5b67e31ec8c
removed.
On Sat, Oct 12, 2013 at 12:57 PM, <dave@cheney.net> wrote:
>
> https://codereview.appspot.com/14532048/diff/6001/ssh/mempipe_test.go
> File ssh/mempipe_test.go (right):
>
> https://codereview.appspot.com/14532048/diff/6001/ssh/mempipe_test.go#newcode89
> ssh/mempipe_test.go:89: func TestmemPipeSafety(t *testing.T) {
> I'd like to see this CL committed so we can move on to the real change.
>
> Please remove this test and propose it as a separate CL so we can debate
> it without blocking the rest of the CL.
>
> https://codereview.appspot.com/14532048/
--
Han-Wen Nienhuys
Google Munich
hanwen@google.com
Message from unknown
2013-10-12T12:51:44+00:00hanwen-googleurn:md5:4e4795cc0b1ceab1ce8591da9773ad94
Message from dave@cheney.net
2013-10-12T12:55:18+00:00dfcurn:md5:af4998026360f87da6c227e699ae265e
thanks, the rest LGTM. ill submit it in the morning if noone else does so first.
Sent from my iPad
On 12/10/2013, at 23:48, Han-Wen Nienhuys <hanwen@google.com> wrote:
> removed.
>
> On Sat, Oct 12, 2013 at 12:57 PM, <dave@cheney.net> wrote:
>>
>> https://codereview.appspot.com/14532048/diff/6001/ssh/mempipe_test.go
>> File ssh/mempipe_test.go (right):
>>
>> https://codereview.appspot.com/14532048/diff/6001/ssh/mempipe_test.go#newcode89
>> ssh/mempipe_test.go:89: func TestmemPipeSafety(t *testing.T) {
>> I'd like to see this CL committed so we can move on to the real change.
>>
>> Please remove this test and propose it as a separate CL so we can debate
>> it without blocking the rest of the CL.
>>
>> https://codereview.appspot.com/14532048/
>
>
>
> --
> Han-Wen Nienhuys
> Google Munich
> hanwen@google.com
Message from dave@cheney.net
2013-10-12T21:34:50+00:00dfcurn:md5:37a213ba4149cde2ee8590594faf1190
*** Submitted as https://code.google.com/p/go/source/detail?r=bb19605bfacc&repo=crypto ***
go.crypto/ssh: implement memTransport using sync.Cond.
This makes memTransport safe for use with multiple
writers/closers.
R=golang-dev, dave
CC=agl, golang-dev
https://codereview.appspot.com/14532048
Committer: Dave Cheney <dave@cheney.net>
Message from hanwen@google.com
2013-10-29T16:13:22+00:00hanwen-googleurn:md5:d92621d37f3ca2c94b7d7ca34ff1e684
*** Abandoned ***