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

Issue 45200043: code review 45200043: go.crypto/ssh: don't block Wait on copying stdin (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 3 months ago by axw
Modified:
10 years, 2 months ago
Visibility:
Public.

Description

go.crypto/ssh: don't block Wait on copying stdin The new TestRunCommandStdin test case would block with the old code, as Wait blocks until the stdio copy goroutines complete. To resolve this, I have added a pipe between the user-specified stdin and the stdin channel. The write end of the pipe is closed prior to waiting for the copy goroutine's completion. Fixes #6088

Patch Set 1 #

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

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+66 lines, -2 lines) Patch
M ssh/session.go View 1 3 chunks +18 lines, -2 lines 0 comments Download
M ssh/test/session_test.go View 1 2 chunks +48 lines, -0 lines 0 comments Download

Messages

Total messages: 3
axw
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go.crypto
10 years, 3 months ago (2013-12-24 04:42:48 UTC) #1
hanwenn
Could you prepare a patch for https://code.google.com/p/gosshnew/ instead? the go.crypto/ssh package is frozen until the ...
10 years, 3 months ago (2013-12-29 23:15:54 UTC) #2
axw
10 years, 2 months ago (2014-01-10 05:39:42 UTC) #3
On 2013/12/29 23:15:54, hanwenn wrote:
> Could you prepare a patch for https://code.google.com/p/gosshnew/ instead? 
> the go.crypto/ssh package is frozen until the refactoring in gosshnew is 
> done.

Done: https://codereview.appspot.com/50380043
 
> On Tuesday, December 24, 2013 5:42:49 AM UTC+1, mailto:axw...@gmail.com wrote:
> >
> > Reviewers: golang-codereviews, 
> >
> > Message: 
> > Hello mailto:golang-co...@googlegroups.com <javascript:>, 
> >
> > I'd like you to review this change to 
> > https://code.google.com/p/go.crypto 
> >
> >
> > Description: 
> > go.crypto/ssh: don't block Wait on copying stdin 
> >
> > The new TestRunCommandStdin test case would block 
> > with the old code, as Wait blocks until the stdio 
> > copy goroutines complete. To resolve this, I have 
> > added a pipe between the user-specified stdin and 
> > the stdin channel. The write end of the pipe is 
> > closed prior to waiting for the copy goroutine's 
> > completion. 
> >
> > Fixes #6088 
> >
> > Please review this at https://codereview.appspot.com/45200043/ 
> >
> > Affected files (+66, -2 lines): 
> >    M ssh/session.go 
> >    M ssh/test/session_test.go 
> >
> >
> > Index: ssh/session.go 
> > =================================================================== 
> > --- a/ssh/session.go 
> > +++ b/ssh/session.go 
> > @@ -137,6 +137,11 @@ 
> >
> >           // true if pipe method is active 
> >           stdinpipe, stdoutpipe, stderrpipe bool 
> > + 
> > +        // stdinPipeWriter is non-nil if StdinPipe was not called 
> > +        // and Stdin is specified by the user; it is the write end 
> > +        // of a pipe connecting Session.Stdin to the stdin channel. 
> > +        stdinPipeWriter io.WriteCloser 
> >   } 
> >
> >   // RFC 4254 Section 6.4. 
> > @@ -396,6 +401,9 @@ 
> >           } 
> >           waitErr := s.wait() 
> >
> > +        if s.stdinPipeWriter != nil { 
> > +                s.stdinPipeWriter.Close() 
> > +        } 
> >           var copyError error 
> >           for _ = range s.copyFuncs { 
> >                   if err := <-s.errors; err != nil && copyError == nil { 
> > @@ -476,11 +484,19 @@ 
> >           if s.stdinpipe { 
> >                   return 
> >           } 
> > +        var stdin io.Reader 
> >           if s.Stdin == nil { 
> > -                s.Stdin = new(bytes.Buffer) 
> > +                stdin = new(bytes.Buffer) 
> > +        } else { 
> > +                r, w := io.Pipe() 
> > +                go func() { 
> > +                        _, err := io.Copy(w, s.Stdin) 
> > +                        w.CloseWithError(err) 
> > +                }() 
> > +                stdin, s.stdinPipeWriter = r, w 
> >           } 
> >           s.copyFuncs = append(s.copyFuncs, func() error { 
> > -                _, err := io.Copy(s.clientChan.stdin, s.Stdin) 
> > +                _, err := io.Copy(s.clientChan.stdin, stdin) 
> >                   if err1 := s.clientChan.stdin.Close(); err == nil && 
> > err1 != io.EOF { 
> >                           err = err1 
> >                   } 
> > Index: ssh/test/session_test.go 
> > =================================================================== 
> > --- a/ssh/test/session_test.go 
> > +++ b/ssh/test/session_test.go 
> > @@ -11,6 +11,7 @@ 
> >   import ( 
> >           "bytes" 
> >           "code.google.com/p/go.crypto/ssh" 
> > +        "errors" 
> >           "io" 
> >           "strings" 
> >           "testing" 
> > @@ -71,6 +72,53 @@ 
> >           } 
> >   } 
> >
> > +func TestRunCommandStdin(t *testing.T) { 
> > +        server := newServer(t) 
> > +        defer server.Shutdown() 
> > +        conn := server.Dial(clientConfig()) 
> > +        defer conn.Close() 
> > + 
> > +        session, err := conn.NewSession() 
> > +        if err != nil { 
> > +                t.Fatalf("session failed: %v", err) 
> > +        } 
> > +        defer session.Close() 
> > + 
> > +        r, w := io.Pipe() 
> > +        defer r.Close() 
> > +        defer w.Close() 
> > +        session.Stdin = r 
> > + 
> > +        err = session.Run("true") 
> > +        if err != nil { 
> > +                t.Fatalf("session failed: %v", err) 
> > +        } 
> > +} 
> > + 
> > +func TestRunCommandStdinError(t *testing.T) { 
> > +        server := newServer(t) 
> > +        defer server.Shutdown() 
> > +        conn := server.Dial(clientConfig()) 
> > +        defer conn.Close() 
> > + 
> > +        session, err := conn.NewSession() 
> > +        if err != nil { 
> > +                t.Fatalf("session failed: %v", err) 
> > +        } 
> > +        defer session.Close() 
> > + 
> > +        r, w := io.Pipe() 
> > +        defer r.Close() 
> > +        session.Stdin = r 
> > +        pipeErr := errors.New("closing write end of pipe") 
> > +        w.CloseWithError(pipeErr) 
> > + 
> > +        err = session.Run("true") 
> > +        if err != pipeErr { 
> > +                t.Fatalf("expected %v, found %v", pipeErr, err) 
> > +        } 
> > +} 
> > + 
> >   func TestRunCommandWeClosed(t *testing.T) { 
> >           server := newServer(t) 
> >           defer server.Shutdown() 
> >
> >
> >
Sign in to reply to this message.

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