-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CHE-3216: Fix kill terminal process and its children on closing terminal. #3395
Conversation
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1324/ |
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1384/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Please address my comments.
@@ -215,13 +215,12 @@ public void onError() { | |||
} | |||
|
|||
/** | |||
* Sends 'exit' command on server side to stop terminal. | |||
* Sends 'kill' command on server side to stop terminal. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It actually closes terminal
*/ | ||
public void stopTerminal() { | ||
if (connected) { | ||
Jso jso = Jso.create(); | ||
jso.addField("type", "data"); | ||
jso.addField("data", "exit\n"); | ||
jso.addField("type", "kill"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please rename to close
if !finalizer.fileClosed { | ||
file.Close() | ||
finalizer.fileClosed = true | ||
fmt.Println("Pty file closed.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also log if closing failes
@@ -141,6 +159,10 @@ func sendConnectionInputToPty(conn *websocket.Conn, reader io.ReadCloser, f *os. | |||
log.Printf("Invalid message %s\n", err) | |||
continue | |||
} | |||
if msg.Type == "kill" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please rename to close
@@ -141,6 +159,10 @@ func sendConnectionInputToPty(conn *websocket.Conn, reader io.ReadCloser, f *os. | |||
log.Printf("Invalid message %s\n", err) | |||
continue | |||
} | |||
if msg.Type == "kill" { | |||
wp.Stop(finalizer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename to close
) | ||
|
||
type wsPty struct { | ||
Cmd *exec.Cmd // pty builds on os.exec | ||
PtyFile *os.File // a pty is simply an os.File | ||
} | ||
|
||
func (wp *wsPty) Stop(finalizer *ReadWriteRoutingFinalizer) { | ||
closeFile(wp.PtyFile, finalizer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please comment this function well as it is not obvious and we will forget why it is implemented in such a way
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1461/ |
…nal. Signed-off-by: Aleksandr Andrienko <aandrienko@codenvy.com>
20f39d4
to
169cb12
Compare
ci-build |
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1574/ |
…nal. (eclipse-che#3395) Signed-off-by: Aleksandr Andrienko <aandrienko@codenvy.com>
What does this PR do?
Create new type websocket message to realise ability to kill terminal process and its child processes when user click "close button".
What issues does this PR fix or reference?
#3216
@evoevodin and @garagatyi review please.
Signed-off-by: Aleksandr Andrienko aandrienko@codenvy.com