Should gosh.Cmd.AddStdoutWriter auto-close the given writer?

[tatatodd]

Adam - I'm only writing this now to avoid forgetting; don't worry about discussing until next week. :)

It bugs me that gosh.Cmd.AddStdoutWriter (and the stderr analog) take an io.Writer argument, but will automatically call Close on the writer if it happens to be an io.WriteCloser when the process finishes. Here are some reasons I think it's bad:

1. The user might also call Close themselves, and there's no guarantee that it's safe to call Close multiple times, either synchronously or concurrently. So in the worst-case we might crash the program with a race. Or in an also-bad-case the first Close might return an error, and be dropped because gosh called it first when the program finished, and then the user might call it and receive a different error, or even success.
   https://golang.org/pkg/io/#Closer

2. As a general rule we should try to avoid type assertions (dynamic typing) when possible, since we lose some of the benefits of static typing, and the resulting behavior may be confusing to users.

This afternoon I tried to to remove this behavior, such that AddStdoutWriter never attempts to close the io.Writer argument. I figured this should be fine, since at the moment that the user is calling AddStdoutWriter, if they actually have an io.WriteCloser in their hands, they can arrange to handle closing it themselves. E.g. this works for for our PrefixLineWriter use-case here:
https://vanadium.googlesource.com/release.go.x.ref/+/master/services/cluster/vkube/vkube_v23_test.go#70

But it didn't work. It was fine for the above use-case, but I had forgotten that we're also relying on the auto-closing behavior of AddStdoutWriter for pipelining. As a recap, if we have a process pipeline "a | b", the user writes the following code:
https://vanadium.googlesource.com/release.go.x.lib/+/master/gosh/shell_test.go#455
a.AddStdoutWriter(b.StdinPipe())
a.Start()
b.Run()

Technically speaking, we can make this work, at the expense of ugly pipelining. E.g. this would work, but I don't like it, and I doubt you'd like it either:
bStdin := b.StdinPipe()
a.AddStdoutWriter(bStdin)
a.Run()
bStdin.Close()
b.Run()

Here are the alternatives I've considered, but I don't like any of them:

A) Remove the auto-closing behavior of AddStd{out,err}Writer, and add AddStd{out,err}WriteCloser as additional methods. The extra methods seem confusing and error-prone for the user.

B) Remove the auto-closing behavior, and add some additional gosh concept to make pipelining even simpler. E.g. perhaps some sort of gosh.Pipeline type or something like that, which makes it even simpler to chain "a | b | c" pipelines together, and doesn't require the user to Start a and b; they can just run the entire pipeline to get the final result. This is hand-wavy, but even if it worked out, I'm wary of adding another Pipeline concept.

C) Keep the existing auto-closing behavior. This has drawbacks (1) and (2) above.

What are your thoughts?

[asadovsky]

Thanks for the detailed and thoughtful writeup. I'll need to give this some thought.

My initial reaction is that (A) is unappealing, and it's not clear whether (B) is worth the bloat, so my inclination is to start by picking between (C) vs. simply dropping the auto-close behavior and making pipelines more awkward - and if the latter, doing (B) later if we decide it's worthwhile.

Regarding (C), I don't think drawbacks (1) and (2) are too severe in practice, especially given that the caller can always wrap their WriteCloser into a noop closer. That said, I agree it's not ideal. (IIRC there's precedent in the standard library for relying on type assertions in these kinds of situations, but I can't recall any specific examples at the moment.)

[tatatodd]

I had a different idea, and sent out some CLs that implement it here:
https://vanadium-review.googlesource.com/#/q/topic:toddw-gosh_close

It's a slightly different idea based on (C):

D) Keep the existing auto-closing behavior, but change the argument from io.Writer to io.WriteCloser. This makes the auto-closing a bit more explicit and expected. Also add gosh.NopWriteCloser which extends a regular io.Writer with an extra no-op Close method.

I like this option. It doesn't suffer from drawback (2). Technically it still suffers from drawback (1), since the user can still call Close themselves. But since we've made it explicit, I think it's a bit easier for the user to discover that this is happening. In addition, the user can use gosh.NopWriteCloser to disable the auto-closing behavior.

FYI there is definitely precedent in the Go std library relying on type assertions. An example is io.Copy, where it uses type assertions to invoke fast-path copying routines:
https://golang.org/pkg/io/#Copy

Thinking about this more, I think the real reason I don't like type-assertions from io.Writer to io.WriteCloser is because of the very loose implementation-defined semantics of Close; that makes the type assertion seem pretty dangerous.

Again, feel free to disregard until next week. :)

[tatatodd]

I've submitted the CLs that implement option (D):
https://vanadium-review.googlesource.com/#/q/topic:toddw-gosh_close