{"action":"created","issue":{"url":"https://api.github.com/repos/vanadium/issues/issues/1031","labels_url":"https://api.github.com/repos/vanadium/issues/issues/1031/labels{/name}","comments_url":"https://api.github.com/repos/vanadium/issues/issues/1031/comments","events_url":"https://api.github.com/repos/vanadium/issues/issues/1031/events","html_url":"https://github.com/vanadium/issues/issues/1031","id":124183207,"number":1031,"title":"Should gosh.Cmd.AddStdoutWriter auto-close the given writer?","user":{"login":"tatatodd","id":5453737,"avatar_url":"https://avatars.githubusercontent.com/u/5453737?v=3","gravatar_id":"","url":"https://api.github.com/users/tatatodd","html_url":"https://github.com/tatatodd","followers_url":"https://api.github.com/users/tatatodd/followers","following_url":"https://api.github.com/users/tatatodd/following{/other_user}","gists_url":"https://api.github.com/users/tatatodd/gists{/gist_id}","starred_url":"https://api.github.com/users/tatatodd/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tatatodd/subscriptions","organizations_url":"https://api.github.com/users/tatatodd/orgs","repos_url":"https://api.github.com/users/tatatodd/repos","events_url":"https://api.github.com/users/tatatodd/events{/privacy}","received_events_url":"https://api.github.com/users/tatatodd/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/vanadium/issues/labels/Area:%20Core","name":"Area: Core","color":"fad8c7"},{"url":"https://api.github.com/repos/vanadium/issues/labels/Lang:%20Go","name":"Lang: Go","color":"5319e7"}],"state":"open","locked":false,"assignee":{"login":"asadovsky","id":538767,"avatar_url":"https://avatars.githubusercontent.com/u/538767?v=3","gravatar_id":"","url":"https://api.github.com/users/asadovsky","html_url":"https://github.com/asadovsky","followers_url":"https://api.github.com/users/asadovsky/followers","following_url":"https://api.github.com/users/asadovsky/following{/other_user}","gists_url":"https://api.github.com/users/asadovsky/gists{/gist_id}","starred_url":"https://api.github.com/users/asadovsky/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/asadovsky/subscriptions","organizations_url":"https://api.github.com/users/asadovsky/orgs","repos_url":"https://api.github.com/users/asadovsky/repos","events_url":"https://api.github.com/users/asadovsky/events{/privacy}","received_events_url":"https://api.github.com/users/asadovsky/received_events","type":"User","site_admin":false},"milestone":null,"comments":1,"created_at":"2015-12-29T08:56:47Z","updated_at":"2015-12-29T18:48:13Z","closed_at":null,"body":"Adam - I'm only writing this now to avoid forgetting; don't worry about discussing until next week.  :)\r\n\r\nIt 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:\r\n\r\n1) 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.\r\nhttps://golang.org/pkg/io/#Closer\r\n\r\n2) 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.\r\n\r\nThis 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:\r\nhttps://vanadium.googlesource.com/release.go.x.ref/+/master/services/cluster/vkube/vkube_v23_test.go#70\r\n\r\nBut 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:\r\nhttps://vanadium.googlesource.com/release.go.x.lib/+/master/gosh/shell_test.go#455\r\n  a.AddStdoutWriter(b.StdinPipe())\r\n  a.Start()\r\n  b.Run()\r\n\r\nTechnically 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:\r\n  bStdin := b.StdinPipe()\r\n  a.AddStdoutWriter(bStdin)\r\n  a.Run()\r\n  bStdin.Close()\r\n  b.Run()\r\n\r\nHere are the alternatives I've considered, but I don't like any of them:\r\n\r\nA) 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.\r\n\r\nB) 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.\r\n\r\nC) Keep the existing auto-closing behavior.  This has drawbacks (1) and (2) above.\r\n\r\nWhat are your thoughts?"},"comment":{"url":"https://api.github.com/repos/vanadium/issues/issues/comments/167851687","html_url":"https://github.com/vanadium/issues/issues/1031#issuecomment-167851687","issue_url":"https://api.github.com/repos/vanadium/issues/issues/1031","id":167851687,"user":{"login":"asadovsky","id":538767,"avatar_url":"https://avatars.githubusercontent.com/u/538767?v=3","gravatar_id":"","url":"https://api.github.com/users/asadovsky","html_url":"https://github.com/asadovsky","followers_url":"https://api.github.com/users/asadovsky/followers","following_url":"https://api.github.com/users/asadovsky/following{/other_user}","gists_url":"https://api.github.com/users/asadovsky/gists{/gist_id}","starred_url":"https://api.github.com/users/asadovsky/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/asadovsky/subscriptions","organizations_url":"https://api.github.com/users/asadovsky/orgs","repos_url":"https://api.github.com/users/asadovsky/repos","events_url":"https://api.github.com/users/asadovsky/events{/privacy}","received_events_url":"https://api.github.com/users/asadovsky/received_events","type":"User","site_admin":false},"created_at":"2015-12-29T18:48:13Z","updated_at":"2015-12-29T18:48:13Z","body":"Thanks for the detailed and thoughtful writeup. I'll need to give this some thought.\r\n\r\nMy 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.\r\n\r\nRegarding (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.)"}}