Do not exit on EOF #45
Conversation
Forward the EOF to GoTTY, and let the server-side decide if it wants to terminate the connection. The server closes the connection, and in response we terminate the readLoop which in turn signals writeLoop to terminate via the QuitChan. This allows for the user to pipe commands to gotty-client, and capture all the result sent by the server. For eg. when gotty is launched as `gotty -w bash`, the following command would now wait to capture all output from the server. for (( i = 0 ; i < 2; ++i )); do echo echo $i; echo sleep 2; done | ./gotty-client https://gotty.example.com Before this patch, gotty-client used to exit on encountering EOF from the left side of the pipe.
|
Hi @gurjeet, it looks great, thanks ! LGTM @QuentinPerez LGTY ? |
|
Hi @gurjeet, thank you for your contribution. 😊 Currently your changes don't work on my mac (it stays on |
|
@QuentinPerez What's the error/problem you see with current state of the PR? I can make that change, but would like to know why making the channel buffered is necessary? From what I can tell, the code should work as expected even with unbuffered channel. FWIW, it works for me and I am using Go version 1.6.3 on MacOS 10.11.6 (El Capitan). |
|
The problem comes from the my patch to show the problem diff --git a/gotty-client.go b/gotty-client.go
index f376a57..57b25fe 100644
--- a/gotty-client.go
+++ b/gotty-client.go
@@ -243,6 +243,7 @@ func (c *Client) Loop() error {
}
case <-c.QuitChan:
}
+ fmt.Println("WAIT")
wg.Wait()
return nil
}
@@ -318,7 +319,9 @@ func (c *Client) writeLoop(done chan bool, wg *sync.WaitGroup) {
err = c.write(append([]byte("0"), byte(4)))
if err != nil {
+ fmt.Println("1: try to send true")
done <- true
+ fmt.Println("1: success")
return
}
continue
@@ -362,7 +365,9 @@ func (c *Client) readLoop(done chan bool, wg *sync.WaitGroup) {
return
case msg := <-msgChan:
if msg.Err != nil {
+ fmt.Println("2: try to send true")
done <- true
+ fmt.Println("2: success")
if _, ok := msg.Err.(*websocket.CloseError); !ok {
logrus.Warnf("c.Conn.ReadMessage: %v", msg.Err)
}
@@ -375,12 +380,12 @@ func (c *Client) readLoop(done chan bool, wg *sync.WaitGroup) {
}
switch msg.Data[0] {
case '0': // data
- buf, err := base64.StdEncoding.DecodeString(string(msg.Data[1:]))
+ _, err := base64.StdEncoding.DecodeString(string(msg.Data[1:]))
if err != nil {
logrus.Warnf("Invalid base64 content: %q", msg.Data[1:])
break
}
- c.Output.Write(buf)
+ // c.Output.Write(buf)
case '1': // pong
case '2': // new title
newTitle := string(msg.Data[1:])and the output $ for (( i = 0 ; i < 2; ++i )); do echo echo $i; echo sleep 2; done | ./gotty-client http://127.0.0.1:8080/
2: try to send true
2: success
WAIT
1: try to send true
# <- can not send true here |
|
Thanks @QuentinPerez! |
|
Per Gurjeet, this seems somewhat fragile. How about setting up a channel that listens for a "poison pill" in the goroutines, i.e. when one dies, they all do. This would be more stable against the addition of additional goroutines to the project, as this proposal is not. |
| func die(fname string, poison chan bool) PosionAction { | ||
| logrus.Info(fname + " died") | ||
|
|
||
| was_open := <-poison |
There was a problem hiding this comment.
don't use underscores in Go names; var was_open should be wasOpen
| Killed | ||
| ) | ||
|
|
||
| func open_poison(fname string, poison chan bool) PosionAction { |
There was a problem hiding this comment.
don't use underscores in Go names; func open_poison should be openPoison
| type PosionAction int | ||
|
|
||
| const ( | ||
| CommittedSuicide = iota |
There was a problem hiding this comment.
exported const CommittedSuicide should have comment (or a comment on this block) or be unexported
| @@ -255,8 +250,47 @@ type winsize struct { | |||
| y uint16 | |||
| } | |||
|
|
|||
| func (c *Client) termsizeLoop(wg *sync.WaitGroup) { | |||
| type PosionAction int | |||
There was a problem hiding this comment.
exported type PosionAction should have comment or be unexported
|
Thanks @davidfetter! @QuentinPerez @moul, I have pushed another commit into this PR. This specifically addresses the fragility of the current method of killing spawned-goroutnes. We now use a "poison-pill" in these goroutines, enabling the exit of any one of them to kill the rest of them cleanly. Can you please try to poke holes in this PR. |
All cooperating goroutines regularly try to read from the shared "poison" channel. If the read succeeds, they exit by calling die(), assuming somebody else cracked open the poison pill. When any of these goroutines is done with its job, it signals other goroutines to exit by calling open_poison() on the shared channel. This approach takes advantage of the fact that reads from a closed channel always succeed. The driving goroutine (Client.Loop() in this case), is called from the "main" goroutine. And because when the "main" goroutine exits, the whole program exit (using os.Exit()) irrepective of liveness of other goroutines, we could not use the same "poison" channel to wait in the driving goroutine. Instead, we use sync.WaitGroup to wait for spawned goroutines, because we want the spawned goroutines to cleanup and exit cleanly.
|
What's the latest on this? |
|
LGTM thank you @gurjeet 😃 |
|
Any chance of a 1.6.1 release with this change included? |
|
Sure, here it is https://github.com/moul/gotty-client/releases/tag/v1.6.1 :) |
Forward the EOF to GoTTY, and let the server-side decide if it wants to
terminate the connection. The server closes the connection, and in
response we terminate the readLoop which in turn signals writeLoop to
terminate via the QuitChan.
This allows for the user to pipe commands to gotty-client, and capture
all the result sent by the server. For eg. when gotty is launched as
gotty -w bash, the following command would now wait to capture alloutput from the server.
Before this patch, gotty-client used to exit on encountering EOF from
the left side of the pipe.