Issue 11781043: code review 11781043: go.crypto/ssh: Use net.UnixConn for connecting client a... (Closed)
|
Created:
10 years, 9 months ago by hanwen-google Modified:
10 years, 7 months ago CC:
dfc, agl1, albert.strasheim, golang-dev Visibility:
Public. |
Descriptiongo.crypto/ssh: Use net.UnixConn for connecting client and sshd.
This obviates custom code to emulate a thread-safe connection.
Use this for testing that listeners close if the connection breaks.
Patch Set 1 #Patch Set 2 : diff -r c0932d3462e8 https://code.google.com/p/go.crypto #
Total comments: 5
Patch Set 3 : diff -r c0932d3462e8 https://code.google.com/p/go.crypto #Patch Set 4 : diff -r c0932d3462e8 https://code.google.com/p/go.crypto #Patch Set 5 : diff -r 1e7a3e301825 https://code.google.com/p/go.crypto #
Total comments: 2
Patch Set 6 : diff -r 5c2f63878818 https://code.google.com/p/go.crypto #Patch Set 7 : diff -r 5c2f63878818 https://code.google.com/p/go.crypto #MessagesTotal messages: 18
Hello dave@cheney.net, agl@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go.crypto
Sign in to reply to this message.
tested with go test -race ; the result is clean.
Sign in to reply to this message.
https://codereview.appspot.com/11781043/diff/2001/ssh/test/forward_unix_test.go File ssh/test/forward_unix_test.go (right): https://codereview.appspot.com/11781043/diff/2001/ssh/test/forward_unix_test.... ssh/test/forward_unix_test.go:176: server.clientConn.Close() I tried closing the server-side too, but for some reason that does not cause the client-side read to exit.
Sign in to reply to this message.
On 2013/07/24 20:22:03, hanwen wrote: > https://codereview.appspot.com/11781043/diff/2001/ssh/test/forward_unix_test.go > File ssh/test/forward_unix_test.go (right): > > https://codereview.appspot.com/11781043/diff/2001/ssh/test/forward_unix_test.... > ssh/test/forward_unix_test.go:176: server.clientConn.Close() > I tried closing the server-side too, but for some reason that does not cause the > client-side read to exit. This usually happens if you have two file descriptors open for the same UNIX socket but only close one of them, e.g. c2 and c2.File(). Socketpair is a bit cleaner than the TempFile approach. Cheers Albert
Sign in to reply to this message.
Very nice, I like the way TryDial will have cleaned up everything on disk before even starting the test. I think this is very close, just a few nits to fix. https://codereview.appspot.com/11781043/diff/2001/ssh/test/test_unix_test.go File ssh/test/test_unix_test.go (right): https://codereview.appspot.com/11781043/diff/2001/ssh/test/test_unix_test.go#... ssh/test/test_unix_test.go:178: s.t.Errorf("unixConnection: %v", err) Fatalf https://codereview.appspot.com/11781043/diff/2001/ssh/test/test_unix_test.go#... ssh/test/test_unix_test.go:184: s.t.Errorf("UnixConn.File: %v", err) s/Errorf/Fatalf
Sign in to reply to this message.
Socketpair has an inherent race condition: socketpair will give you 2 file descriptors, and if you can only do after the read syscall kicks off, but there is no way to synchronize on that without help from the network poller. I guess it uses conn.File() dup under the hood? Let me try to close both and see if it works On Wed, Jul 24, 2013 at 10:26 PM, <fullung@gmail.com> wrote: > On 2013/07/24 20:22:03, hanwen wrote: > > https://codereview.appspot.com/11781043/diff/2001/ssh/test/forward_unix_test.go >> >> File ssh/test/forward_unix_test.go (right): > > > > https://codereview.appspot.com/11781043/diff/2001/ssh/test/forward_unix_test.... >> >> ssh/test/forward_unix_test.go:176: server.clientConn.Close() >> I tried closing the server-side too, but for some reason that does not > > cause the >> >> client-side read to exit. > > > This usually happens if you have two file descriptors open for the same > UNIX socket but only close one of them, e.g. c2 and c2.File(). > > Socketpair is a bit cleaner than the TempFile approach. > > Cheers > > Albert > > https://codereview.appspot.com/11781043/ -- Han-Wen Nienhuys Google Munich hanwen@google.com
Sign in to reply to this message.
Sorry - not enough coffee. On Thu, Jul 25, 2013 at 10:13 AM, Han-Wen Nienhuys <hanwen@google.com> wrote: > Socketpair has an inherent race condition: socketpair will give you 2 > file descriptors, and if you can only do after the read syscall kicks you can only close from thread 2 after the read syscall is kicked off in thread 1. > off, but there is no way to synchronize on that without help from the > network poller. > > I guess it uses conn.File() dup under the hood? Let me try to close > both and see if it works > > On Wed, Jul 24, 2013 at 10:26 PM, <fullung@gmail.com> wrote: >> On 2013/07/24 20:22:03, hanwen wrote: >> >> https://codereview.appspot.com/11781043/diff/2001/ssh/test/forward_unix_test.go >>> >>> File ssh/test/forward_unix_test.go (right): >> >> >> >> https://codereview.appspot.com/11781043/diff/2001/ssh/test/forward_unix_test.... >>> >>> ssh/test/forward_unix_test.go:176: server.clientConn.Close() >>> I tried closing the server-side too, but for some reason that does not >> >> cause the >>> >>> client-side read to exit. >> >> >> This usually happens if you have two file descriptors open for the same >> UNIX socket but only close one of them, e.g. c2 and c2.File(). >> >> Socketpair is a bit cleaner than the TempFile approach. >> >> Cheers >> >> Albert >> >> https://codereview.appspot.com/11781043/ > > > > -- > Han-Wen Nienhuys > Google Munich > hanwen@google.com -- Han-Wen Nienhuys Google Munich hanwen@google.com
Sign in to reply to this message.
I tried closing the server side too, but couldn't get it to work. https://codereview.appspot.com/11781043/diff/2001/ssh/test/test_unix_test.go File ssh/test/test_unix_test.go (right): https://codereview.appspot.com/11781043/diff/2001/ssh/test/test_unix_test.go#... ssh/test/test_unix_test.go:178: s.t.Errorf("unixConnection: %v", err) On 2013/07/25 11:18:08, dfc wrote: > Fatalf Done. https://codereview.appspot.com/11781043/diff/2001/ssh/test/test_unix_test.go#... ssh/test/test_unix_test.go:184: s.t.Errorf("UnixConn.File: %v", err) On 2013/07/25 11:18:08, dfc wrote: > s/Errorf/Fatalf Done.
Sign in to reply to this message.
ping?
Sign in to reply to this message.
Ping? Adam, you have an opinion?
Sign in to reply to this message.
LGTM https://codereview.appspot.com/11781043/diff/27001/ssh/test/test_unix_test.go File ssh/test/test_unix_test.go (right): https://codereview.appspot.com/11781043/diff/27001/ssh/test/test_unix_test.go... ssh/test/test_unix_test.go:165: listener.Close() This appears to duplicate the defer on line 154.
Sign in to reply to this message.
https://codereview.appspot.com/11781043/diff/27001/ssh/test/test_unix_test.go File ssh/test/test_unix_test.go (right): https://codereview.appspot.com/11781043/diff/27001/ssh/test/test_unix_test.go... ssh/test/test_unix_test.go:165: listener.Close() On 2013/08/28 16:28:52, agl1 wrote: > This appears to duplicate the defer on line 154. Done.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=5d03cdb555ef&repo=crypto *** go.crypto/ssh: Use net.UnixConn for connecting client and sshd. This obviates custom code to emulate a thread-safe connection. Use this for testing that listeners close if the connection breaks. R=dave, agl, fullung CC=golang-dev https://codereview.appspot.com/11781043 Committer: Adam Langley <agl@golang.org>
Sign in to reply to this message.
*** Abandoned ***
Sign in to reply to this message.
Doh, I missed this one. Does net.Pipe() not work?
Sign in to reply to this message.
with net.Pipe, the thing deadlocks on shutdown. I guess some part of the piping needs to be closed before Wait()ing, but did not look more closely. On Thu, Aug 29, 2013 at 12:20 AM, <jpsugar@google.com> wrote: > Doh, I missed this one. Does net.Pipe() not work? -- Han-Wen Nienhuys Google Munich hanwen@google.com
Sign in to reply to this message.
Oh, I see. You can't use net.Pipe because it doesn't support File(). And I suppose syscall.Socketpair isn't portable. Okay, thanks. On Thu, Aug 29, 2013 at 4:57 AM, Han-Wen Nienhuys <hanwen@google.com> wrote: > with net.Pipe, the thing deadlocks on shutdown. I guess some part of > the piping needs to be closed before Wait()ing, but did not look more > closely. > > On Thu, Aug 29, 2013 at 12:20 AM, <jpsugar@google.com> wrote: >> Doh, I missed this one. Does net.Pipe() not work? > > > > -- > Han-Wen Nienhuys > Google Munich > hanwen@google.com
Sign in to reply to this message.
For the record -- the parts of the pipe that you pass to the subprocess have to be closed in the parent. r2.Close() w1.Close() would probably have fixed it. On Thu, Aug 29, 2013 at 4:57 AM, Han-Wen Nienhuys <hanwen@google.com> wrote: > with net.Pipe, the thing deadlocks on shutdown. I guess some part of > the piping needs to be closed before Wait()ing, but did not look more > closely. > > On Thu, Aug 29, 2013 at 12:20 AM, <jpsugar@google.com> wrote: >> Doh, I missed this one. Does net.Pipe() not work? > > > > -- > Han-Wen Nienhuys > Google Munich > hanwen@google.com
Sign in to reply to this message.
|
