Not receiving `OO` causes session to get stuck

[sorenisanerd]

It seems like a relatively common problem that the terminating OO from sz gets lost or misparsed or something. See all these issues:
sorenisanerd/gotty#46
tsl0922/ttyd#239
tsl0922/ttyd#279
Eugeny/tabby#6259
cferdinandi/tabby#132
Eugeny/tabby#5196
trzsz/tabby-trzsz#2
Eugeny/tabby#5132

This lets me call .allow_missing_OO(true) on the session object which SEEMS to make the problem go away:

diff --git a/src/zsession.js b/src/zsession.js
index 5f0b8f9..cafe57f 100644
--- a/src/zsession.js
+++ b/src/zsession.js
@@ -193,6 +193,10 @@ Zmodem.Session = class ZmodemSession extends _Eventer {
         //console.warn("Invalid first Zmodem header", hdr);
     }
 
+    allow_missing_OO(flag) {
+        this._allow_missing_OO = !!flag;
+    }
+
     /**
      * Sets the sender function that a Session object will use.
      *
@@ -549,7 +553,7 @@ Zmodem.Session.Receive = class ZmodemReceiveSession extends Zmodem.Session {
             if (this._input_buffer.length < 2) return;
 
             //if it’s OO, then set this._bytes_after_OO
-            if (Zmodem.ZMLIB.find_subarray(this._input_buffer, OVER_AND_OUT) === 0) {
+            if (this._allow_missing_OO || Zmodem.ZMLIB.find_subarray(this._input_buffer, OVER_AND_OUT) === 0) {
 
                 //This doubles as an indication that the session has ended.
                 //We need to set this right away so that handlers like

...but I can't reliably trigger the problem, so my confidence in this approach is not great.

Do you have thoughts on how to address this? The spec suggests that if the OO does not arrive, you should just move on with your life after a brief timeout, so maybe we just need to implement that timeout? Just getting stuck in this state doesn't benefit anyone.

[FGasper]

Yeah a timeout seems reasonable; all the same, the terminating OO should come since this is all TCP.

[sorenisanerd]

It very much should! I have spent hours and hours trying to pinpoint how they get lost, but to no avail.

…

On Tue, Oct 18, 2022 at 4:58 PM Felipe Gasper ***@***.***> wrote: Yeah a timeout seems reasonable; all the same, the terminating OO *should* come since this is all TCP. — Reply to this email directly, view it on GitHub <#33 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABHCWXMFXBW6WRQ6LGFCBLWD42RPANCNFSM6AAAAAARIRSPRM> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[FGasper]

I wonder if it’s a bug in sz. Maybe it neglects to flush its output buffer or something.

Maybe _consume_ZFIN() could be where the timeout starts, at the end of which this._on_session_end() would be called and _bytes_after_OO gets set to an empty array … and if the usual OO arrives then we clear the timeout.

[sorenisanerd]

I suspected `sz`, too. I've stared at it a LOT today and it doesn't seem to be doing anything out of the ordinary. https://github.com/UweOhse/lrzsz/blob/master/src/lsz.c#L1855-L1857 is pretty standard. I haven't been able to trigger it if I run either `sz`, `bash`, or `gotty` under `strace` :( Either way, if `sz` is broken, we still need to support current versions of it.

[destinytaoer]

i have same problem. when i use sz to download file, sometimes it return ZFIN at the end as described in the picture above, but sometimes it doesn't return and stuck my terminal. it more frequent occurrences when the file is larger than 100MB

it stucks the tty, not my frontend terminal. maybe it is a problem of lrzsz? what can i do for it?

In my computer, it works fine. but when i use it in k8s exec api to access a pod, that error comes

[sorenisanerd]

I was directed to tsl0922/ttyd@eca54c7 as a fix.

@FGasper, thoughts?

[FGasper]

@sorenisanerd That patch converts the exception into a warning, but how would that affect a hang?

It seems like there are 2 issues:

1. “unexpectedness” after ZFIN … this will trigger the exception
2. blackholed OO, causing a hang

Re #1: going by the issues you’ve linked—thank you!—at first blush I’m wondering if there’s not faulty calling logic somewhere. (zmjs is, alas, all too easy to misuse.) Have you reproduced this?

Is #2 still an issue? If so, how would the patch from ttyd fix it?

[FGasper]

Going by this, a timeout would seem an appropriate fix for #2.