No progress report on sending
sorenisanerd opened this issue · 6 comments
Zmodem.Browser.send_files allows you to pass an on_progress handler. Sadly, it isn't actually usable for a couple of reasons.
First of all, it seems Chrome (and possibly others, I haven't checked) only triggers the progress handler when it's done loading the file. I've tried with files up to ~25 MB in size. Honestly, this makes sense, since on any modern system, loading a file into memory is very fast.
Second, I'm not very interested in learning about the progress in loading the file. I'm much more interested in learning about the progress in sending the file. However, the end_files routine sends as much data as it has available without splitting it into smaller chunks. Due to the first problem, this means the whole file gets sent in one go and my first progress event is when it's done.
I propose adding a chunkSize option to send_files. Files will get split into chunks of up to chunkSize and the on_progress handler should get triggered for each chunk sent.
@sorenisanerd Yeah I realized back when first writing zmjs that the on_progress handler wasn’t all that useful.
ZMODEM can send data a few different ways. This library, since it assumes a reliable transport underneath, uses the option that forgoes error detection. In order for your chunkSize idea to bear the fruit you envision, I think we’d also need to adjust zmodem.js to do “error detection” so we have an indicator from the peer of how far we’ve gotten. That would slow things down but might also be useful to prevent zmodem.js from locking the thread (which I’ve seen happen).
The WebSocket interface does expose bufferedAmount, but things besides ZMODEM would be caught up in that, so that may not work very well. Ideally WebSocket .send() would return a promise that fulfills once the message is sent, but alas, that’s not reality.
I was working on a patch for this end and eventually learned about how the websocket API seems to have an infinite buffer, so we not only load the file almost instantly but also shove it into the websocket buffer almost instantly. Two steps forward, and one step back.
Like you suggest, requesting acknowledgment from the remote seems like the way forward. I tried simply changing the subpacket frame end for transfers from no_end_no_ack/ZCRCG to end_ack/ZCRCW in _send_interim_file_piece, but zmodem.js seemed unhappy about the ZACKs from the other end.
I'll make do without progress indicators for now, but it would be real nice from a UX perspective.
It would be real nice from a UX perspective.
I definitely agree! That said, as it happens I myself don’t have much use for it, so I’m not planning to work on it. I’d merge a good PR that includes suitable tests, though.
I envision something like: every 100th (10th? 1,000th?) ZMODEM subpacket requests asynchronous acknowledgement. That way there should be barely any perceptible impact on transmission speed, but there’ll at least be periodic progress indicators.
Alternatively, implement it with synchronous acknowledgement requests. That’ll impact transmission speed but also prevent zmodem.js from blocking the JavaScript thread, which is what I’ve seen happen when sending large files with it. (It might be a nice improvement—and maybe not too hard?—to convert the ZDLE conversion logic to WebAssembly.)
I think synchronous is better for once. The max size of the websocket buffer is undefined and if it overflows, the connection is terminated unconditionally (per the spec).
I hadn't considered that the request for ACK is a per subpacket thing. Would it make sense to allow a callback function on the session or offer object so users can decide for themselves if they want to request a synchronous ack? Personally, I'd make it depend on the websocket's bufferedAmount.
Note, though, that zmodem.js—by design—doesn’t assume WebSocket is involved. The library internals are transport-agnostic, which facilitates use in Electron apps and the like. So depending on bufferedAmount won’t work.
Yep, that's precisely why I was suggesting a callback option, so WebSockets aren't exposed to zmodem.js internals. 👍