Returning SSH client command stdout and deferred close on SSH resources within a function call

I was working on some code that returns back an io.Reader for data extraction rather than the full output as a slice of bytes which would take up more memory for very large data.

For this question, it is posed against SSH client interface, but could apply to any reader whose underlying resources it comes from has some resources being closed at some point.

The sample test/demo code: Go Playground - The Go Programming Language. It runs locally on my computer, and runs in Go Playground, though sometimes the playground sandbox environment execution will fail. I tested code against real SSH connection earlier also rather than this demo code using test/mock SSH server, showing this code here as is easier to demo with.

package main

import (
	"fmt"
	"io"
	"net"

	"github.com/metarsit/sshtest"
	"golang.org/x/crypto/ssh"
)

func main() {
	addr := "localhost:2222"
  // for real demo, data should ideally be some very large string or array of bytes in size of hundreds of MB
	data := "supposedly some very large data being streamed for I/O processing"

	// init dummy test server to interface to SSH client related code being tested
	hp := sshtest.NewHoneyPot(addr)

	// Start the server in the background
	go func() {
		hp.ListenAndServe()
	}()
	defer hp.Close()

	hp.SetReturnString(data)

	// init the SSH client dependency to test exec cmd & getting the output stream
	cfg := &ssh.ClientConfig{
		User: "jdoe",
		Auth: []ssh.AuthMethod{
			ssh.Password("secret"),
		},
		HostKeyCallback: ssh.HostKeyCallback(
			func(hostname string, remote net.Addr, key ssh.PublicKey) error {
				return nil
			},
		),
	}

	outs, err := runSshCommand(addr, cfg, "echo \"hello world!\"")
	if err != nil {
		fmt.Printf("%v\n", err)
    return
	}
	//result, err := io.ReadAll(outs)
  result := make([]byte, 20) // demo arbitrary partial read against reader
  // NOTE/TODO: what happens when fully reading the returned "outs" reader
  // takes a long time? Can the SSH session or client connection close
  // on remote end and cause stream read failure?
  //
  // And more importantly do the deferred close within the RunSshCommand
  // for session and client affect reading of the returned stream at the caller
  // side after the function has already returned but reader hasn't been
  // completely read yet? Seems no effect the latter case from this simple
  // demo here?
  _, err = io.ReadFull(outs, result)
	if err != nil {
		fmt.Printf("%v\n", err)
    return
	}
	fmt.Printf("main/caller output:\n%s\n", result)
}

func runSshCommand(addr string, cfg *ssh.ClientConfig, cmd string) (io.Reader, error) {

  client, err := ssh.Dial("tcp", addr, cfg)
	if err != nil {
		return nil, fmt.Errorf("Create client failed %v", err)
	}
	defer client.Close()

	// open session
	session, err := client.NewSession()
	if err != nil {
		return nil, fmt.Errorf("Create session failed %v", err)
	}
	defer session.Close()

	stderr, err := session.StderrPipe()
	if err != nil {
		err = fmt.Errorf("cannot open stderr pipe for cmd '%s': %s", cmd, err)
		return nil, err
	}

	stdout, err := session.StdoutPipe()
	if err != nil {
		err = fmt.Errorf("cannot open stdout pipe for cmd '%s': %s", cmd, err)
		return nil, err
	}

	err = session.Run(cmd)
	if err != nil {
		err = fmt.Errorf("cannot run cmd '%s': %s", cmd, err)
		return nil, err
	}

	combinedOutputStream := io.MultiReader(stdout, stderr)

	return combinedOutputStream, nil
}

I was unsure the outcome of the code (same thoughts as my colleague that we’ll get to in a bit) when I was first working with it but the sample code works. When I posted similar code for review at work, a colleague posed question that the io.Reader’s underlying source is stdout from SSH session, that the called function has deferred close statements on the SSH resources (session, and client connection), and on function exit from the deferred closes, wouldn’t the associated stdout linked to the returned reader be closed (and thus we would fail to read the data)?

So the questions for code review here are:

  • why does the sample code still work? Did my colleague & I have mistaken assumptions about how stdout operates relative to the SSH client session?
  • under what conditions would the code not work? How to alter the example to highlight problem cases?

I’m assuming one train of thought would be to return or pass back references to the resources being closed so that you close them as needed when done reading the data from the associated reader or on erroring out rather than deferring the close within the function being called, which would make you think turns the reader out of scope when passed back to the caller. But I would think doing it this way could get cumbersome and complicated for the caller to do, to also have to manage the closes and possibly do kind of async processing. All this wouldn’t be something to worry about in the simplified case of where function simply reads all the stdout data and returns a slice of bytes instead of the reader, but this is at expense of using up memory for the simplification.

What is go best practice for something like this when you want to read/transfer a lot of data (like over SSH) but don’t want to use up memory (or temp files and disk space) in doing so, where we try to use other means like the io.Reader interface. Am I heading in right direction? Or there are other ways to do it or enhancements I could make here?


	combinedOutputStream := io.MultiReader(stdout, stderr)

	io.Copy(os.Stdout, combinedOutputStream)

	return combinedOutputStream, nil

By inserting this paragraph, I think you should understand the answer. What you actually execute is ssh user@host cmd. After execution, the output will end.

As for the problem of reading and generating big data, of course it is forwarding and whoever uses it spends the money, rather than solving this problem by ourselves. For example, when parsing a video, is it difficult to store each frame cost-effectively? Impossible.
If stream data is generated, it should be treated as stream overhead, such as io.Copy to other consumer connections, and excessive buffering should not be stored locally. This is the basic thinking of network programming. Do not store too many buffers in the application layer, because the kernel layer has its own set of buffers (such as tcp). Flow pressure should be placed at the kernel level as much as possible.

Can you please clarify/elaborate a bit? It’s not exactly clear to me your example & description, so wanted to confirm.

You mean to say that the returned (reference to the) io.Reader (the multireader) causes the SSH session to remain open until the data in that pipe is fully consumed (your io.Copy example? which wasn’t in my code), or at least until SSH connection/session closed on the remote end, whichever occurs sooner? If so, this means that the deferred close of the session and the client within the function that has since returned (or exited scope from returning the reader) has no impact or is in effect cancelled by the fact that the reader has not been consumed at the time the function goes out of scope? That is what I’m confused about and wanted confirmation. The defers are tied to the scope they are in, which is the function that returns the reader for consumption outside of the function.

Or do you mean to say (from your next paragraph) that the SSH session and connection has indeed been closed from the function call returning, but that the OS/network layer has somehow cached that data for processing locally so it’s abstracted from the go code? This I can somewhat understand for the additional unseen buffering, but only for the simple example I presented where the data buffered is minimal. It’s harder to grasp the design of this for the case of big data flowing through the SSH connection like say a file transfer of MB or GB size that takes on order of minutes to an hour to complete - say this big data is in the returned “combinedOutputStream” to be consumed, and the returning function has the deferred close - when does the SSH session close per the go code? Or the OS/network abstracts the actual closing timeframe away from the go code? Sorry I didn’t set up a sample demo code for this big data case to test with.

Does my question / code behavior relate to these?

in the sense that between my code as client and the remote server side, holding onto the stdout/reader reference keeps the connection active on SSH client side regardless of the session/client close(), until the reader’s data has been fully read (or goes out of scope from any code execution/reference)?

emmm? You give me the impression that I haven’t used the “ssh user@host cmd” command, and that the output pipe is automatically closed when the cmd is executed.
As for the issues you mentioned, they are not directly related to your question.

Compared to SSH, I think what you’re missing is the logic of stream processing.
For example, if you want to send a file via TCP, what will your program do? Isn’t it possible to read part of the file data through a buf and write it to tcpconn?
In the same way, the size of the output of the SSH far end is indeterminate, which is a logic for sending files. If you are unsure of the scale or do not want to land data inside the program, then you should make sure that the consumer side starts processing the data, rather than processing the data before deciding on the consumer side.
A good processing logic looks something like this:

    srcStream // src data stream
    dstStream // target obj stream
    buf // may be size 1024 []byte
    for {
        srcStream -> buf
        buf -> dstStream
    }

Well, per your prior post for the inserted code you provided,I don’t see anything related to SSH there. Code refers to one thing while the paragraph under talks about something else. They could be related but it’s not intuitive to say a beginner looking at the code and the text below.

Anyways, maybe we’re miscommunicating the topic. I did ask about a wider topic scope over the course of discussion, that may deviated the discussions, my bad. But the core of the question is specific to the go code handling and not about the overall interaction of local client (the go code) and remote SSH server end, nor handling of big data, those just relate to it.

The core question being asked is the deferred close on SSH session and client seem to have no impact on the (yet unread/unused) stdout related reader being returned as the called function goes out of scope. Is the scope of the SSH session in the go code actually tied to the use of stdout (wherever it is used/read) instead of the function being called where SSH session was initiated?

What you said is very confusing. I can only explain what I think your confusion is. This is essentially a problem of stream processing.
session.StderrPipe() and session.StdoutPipe() are generated by session. In your code, you called “defer session.Close()”. When you jumped out of the runSshCommand function, you had closed all the pipes, which means that the io.Reader you returned has no meaning.
When the parent object is closed, the child functions are naturally unavailable. This design is reasonable.
According to your runSshCommand function, the slightly modified approach should be like this:

func runSshCommand2(addr string, cfg *ssh.ClientConfig, cmd string, dst io.Writer) error {
	client, err := ssh.Dial("tcp", addr, cfg)
	if err != nil {
		return fmt.Errorf("Create client failed %v", err)
	}
	defer client.Close()

	// open session
	session, err := client.NewSession()
	if err != nil {
		return fmt.Errorf("Create session failed %v", err)
	}
	defer session.Close()

	stderr, err := session.StderrPipe()
	if err != nil {
		err = fmt.Errorf("cannot open stderr pipe for cmd '%s': %s", cmd, err)
		return err
	}

	stdout, err := session.StdoutPipe()
	if err != nil {
		err = fmt.Errorf("cannot open stdout pipe for cmd '%s': %s", cmd, err)
		return err
	}

	err = session.Run(cmd)
	if err != nil {
		err = fmt.Errorf("cannot run cmd '%s': %s", cmd, err)
		return err
	}

	combinedOutputStream := io.MultiReader(stdout, stderr)

	_, err = io.Copy(dst, combinedOutputStream)
	return err
}

I’m not sure what you want to explore, but ssh belongs to network programming, so it also conforms to the following logic: (Assume that the connection used by ssh is tcpconn)

  1. When the underlying connection is disconnected or abnormal, the upper layer wrapper will also be disconnected or abnormal (for example, tcpconn is disconnected, and the ssh connection is also disconnected)
  2. When reading ssh connection data, it will trigger downward reading of tcpconn data.
  3. A good network library will not read data of unknown size from the bottom layer into the buffer in advance.
  4. When you close the upper connection, you do not necessarily close the lower connection, because it may be multiplexed. So the handler will directly discard the connection data about the upper layer being closed.
  5. There are many more, but this is all I can think of at the moment.

Yes, that was my understanding as well. The confusion for me (hence this forum post) is if you actually run the sample code, the returned reader outside of that function is not nil and can be read without it returning error or EOF right away. This is before your suggested test code modifications. Why it is that way is what I wanted to know, but perhaps that question then should go to the go ssh library developers to get clarification on behavior.

Thanks for the modified code suggestion. In my case, want to keep the interface to a reader being given to caller/user though not a writer. For the suggested code that still works but the user now has to deal with how to turn the writer to a reader (if they needed it that way). So if we wrap that in the function, then I suppose would need to use io.Pipe to copy to the pipe writer and then return the pipe reader, using go routine to prevent blocking between the pipe writer & reader.

Wait, I took a look at the source code of ssh, which is very interesting.
What you are using is session.Run(cmd). It will wait until cmd execution is completed or an exception occurs before it ends blocking. At the same time, because you are using stderr, err:= session.StderrPipe() and stdout, err:= session.StdoutPipe. () directly uses an internal buffer. The description is of linked list type.
This means that your function will internally store the output data in a buffer.
According to your needs, an effective way of writing it should be like this:

func runSshCommand(addr string, cfg *ssh.ClientConfig, cmd string) (r io.ReadCloser, err error) {
	client, err := ssh.Dial("tcp", addr, cfg)
	if err != nil {
		return nil, fmt.Errorf("Create client failed %v", err)
	}
	defer func() {
		if err != nil {
			_ = client.Close()
		}
	}()

	// open session
	session, err := client.NewSession()
	if err != nil {
		return nil, fmt.Errorf("Create session failed %v", err)
	}
	defer func() {
		if err != nil {
			_ = session.Close()
		}
	}()

	reader, writer := io.Pipe()
	session.Stdout = writer
	session.Stderr = writer

	r = &_reader{ReadCloser: reader, s: session}

	err = session.Start(cmd)
	if err != nil {
		err = fmt.Errorf("cannot run cmd '%s': %s", cmd, err)
		return nil, err
	}
	go func() {
		defer client.Close()
		defer session.Close()
		_ = writer.CloseWithError(session.Wait())
	}()
	return r, nil
}

type _reader struct {
	io.ReadCloser
	s *ssh.Session
}

func (r *_reader) Close() error {
	if r.s != nil {
		_ = r.s.Close()
	}
	return r.ReadCloser.Close()
}