Fix stop working terminal widgets when user clicks on the exited task widget.

[AndrienkoAleksandr]

What does this PR do?

Fix stop working terminal widgets when user clicks on the exited task widget.

What issues does this PR fix or reference?

eclipse-che/che#16601

Signed-off-by: Oleksandr Andriienko oandriie@redhat.com

[che-bot]

✅ E2E Happy path tests succeed 🎉

See Details

• Jenkins job

• logs and configs

• Happy path tests DevFile

• images:

name	link
che-theia	docker.io/maxura/che-theia:729
che-theia-endpoint-runtime-binary	docker.io/maxura/che-theia-endpoint-runtime-binary:729

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

[RomanNikitenko]

Looks like typeof this.terminalId !== 'number' is redundant, because on the next step we have IBaseTerminalServer.validateId(this.terminalId) which does the same
wdyt?

Regarding this.processGone I wonder if we have another indicator to avoid using boolean flag. I'll check and let you know.

[RomanNikitenko]

@AndrienkoAleksandr
I see that you defined this.processGone = true for:

• onTerminalExecExit
• onTerminalExecError

cases.

For these cases do we still have alive this.termServer? Can this.termServer be used like this this.termServer!.check({ id: id } for example
if onTerminalExecExit or onTerminalExecError is happen?

[AndrienkoAleksandr]

For these cases do we still have alive this.termServer?

Yes, we do.

[AndrienkoAleksandr]

But, I applied boolean flag, because I don't wan't to make request per each resize. Resize operation is expansive and we could affect performance if we will do to much requests.

[RomanNikitenko]

You are right about performance, I asked because I was looking for a solution without boolean flag.

At the moment we have isOpen flag.
It's switched to true when a socket is open, but is NOT switched to false when:

• onError event
• onClose event
• at disposing for example.

If I understand correctly this.closeOutputConnectionDisposable.dispose() is executed for onTerminalExecExit and onTerminalExecError events, so next a socket will be closed.

If so, can we:

• use isOpen flag instead the new flag
  or
• have socket as field and do this.socket = undefined when a socket is closed. For this case maybe we could remove both boolean flags and use check if socket exist.

I don't know this part of code very well, so sorry if I generate bad ideas :-)
I don't have a goal to avoid using boolean flags at all costs and it's OK to me merge the PR as is if we don't have a better solution at the moment.

Thanks in advance!

[AndrienkoAleksandr]

Hello @RomanNikitenko, thanks a lot for feedback. I removed flag isOpen it was really confused flag with bad name. About this.socket - I think we should not use it to determine process alive or it is gone, because this socket is responsible to send/receive data to the exec, but it is not responsible to make resize.

[che-bot]

✅ E2E Happy path tests succeed 🎉

See Details

• Jenkins job

• logs and configs

• Happy path tests DevFile

• images:

name	link
che-theia	docker.io/maxura/che-theia:729
che-theia-endpoint-runtime-binary	docker.io/maxura/che-theia-endpoint-runtime-binary:729

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

[RomanNikitenko]

Tested using steps to reproduce.
I can reproduce the issue for a stack without these changes.
These changes fix the issue for me!

Thanks, @AndrienkoAleksandr !

[RomanNikitenko]

@AndrienkoAleksandr
btw, there is some problem with ci docker build, please take a look.
maybe restart will help...

[AndrienkoAleksandr]

crw-ci-test

[che-bot]

✅ E2E Happy path tests succeed 🎉

See Details

• Jenkins job

• logs and configs

• Happy path tests DevFile

• images:

name	link
che-theia	docker.io/maxura/che-theia:729
che-theia-endpoint-runtime-binary	docker.io/maxura/che-theia-endpoint-runtime-binary:729

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

[AndrienkoAleksandr]

crw-ci-build