diff options
| -rw-r--r-- | packages/meshbay-hub/src/meshbay_hub/static/file-utils.js | 37 | ||||
| -rw-r--r-- | packages/meshbay-hub/tests/test_downloads.py | 124 |
2 files changed, 129 insertions, 32 deletions
diff --git a/packages/meshbay-hub/src/meshbay_hub/static/file-utils.js b/packages/meshbay-hub/src/meshbay_hub/static/file-utils.js index 065079b..ef074dd 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/file-utils.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/file-utils.js @@ -218,16 +218,42 @@ async function _openDownloadTarget(filename, size = 0, pickerOpts = {}, // // So the queue is explicit now, and it is the *targets* that queue, not the // rows: every download still appears the moment it is asked for. +// +// Two things keep the queue from becoming the problem it was meant to solve. +// It only ever holds openings that could actually put a dialog on screen, and +// no opening waits behind another for longer than a budget. let _targetQueue = Promise.resolve(); let _targetsInFlight = 0; +// How long an opening waits for the one ahead of it before going anyway. +// +// A queue with no bound is a way for one stuck opening to freeze every later +// download for the life of the page, since `_targetQueue` is never reset. That +// is what turned a slow first download into four rows stuck at "preparing" on +// Firefox. Generous, because a dialog legitimately waits for a person and +// cutting in front of one would be worse than waiting; finite, because the +// alternative is a download panel that never recovers. +// +// Going anyway is safe: whatever was ahead is still the only unbatched opening, +// so the one released here takes the streamed path and opens no second dialog. +const TARGET_QUEUE_BUDGET_MS = 90000; + function _openTargetInTurn(filename, size, pickerOpts, swSize) { + // Only an opening that could show a dialog has any reason to wait. Firefox + // and Safari have no `showSaveFilePicker` at all, so nothing there can race + // anything, and queueing them bought nothing while costing everything: four + // downloads that used to open their targets at the same time became four + // that waited on the slowest. + const canPick = typeof window !== 'undefined' + && typeof window.showSaveFilePicker === 'function'; + if (!canPick) return _openDownloadTarget(filename, size, pickerOpts, swSize); + // Anything that has to wait its turn is, by definition, not the first of the // batch — so it will not be the one holding the user's gesture. const batched = _targetsInFlight > 0; _targetsInFlight += 1; - const mine = _targetQueue + const mine = _waitBriefly(_targetQueue, TARGET_QUEUE_BUDGET_MS) .then(() => _openDownloadTarget(filename, size, pickerOpts, swSize, { batched })) .finally(() => { _targetsInFlight -= 1; }); @@ -237,6 +263,15 @@ function _openTargetInTurn(filename, size, pickerOpts, swSize) { return mine; } +/** Settles with `promise`, or after `ms`, whichever comes first. */ +function _waitBriefly(promise, ms) { + return new Promise((resolve) => { + const timer = setTimeout(resolve, ms); + promise.then(() => { clearTimeout(timer); resolve(); }, + () => { clearTimeout(timer); resolve(); }); + }); +} + /** The download of last resort, for browsers with no way to stream to disk. */ function _saveBlob(blob, filename) { const url = URL.createObjectURL(blob); diff --git a/packages/meshbay-hub/tests/test_downloads.py b/packages/meshbay-hub/tests/test_downloads.py index 81ab9d3..dfb1433 100644 --- a/packages/meshbay-hub/tests/test_downloads.py +++ b/packages/meshbay-hub/tests/test_downloads.py @@ -368,6 +368,52 @@ def test_the_worker_is_kept_alive_while_it_streams(): assert "mbdl-ping" in sw and "mbdl-pong" in sw +def _turn_harness(tmp_path, name, body, *, picker=True, budget_ms=90000): + """Run the real `_openTargetInTurn` against a stubbed opener. + + Both it and `_waitBriefly` are lifted out of `file-utils.js` as text; only + the budget is supplied here, so a case about the budget need not wait a + minute and a half for it. + """ + src = (STATIC / "file-utils.js").read_text() + + def lift(decl): + cut = src[src.index(decl):] + return cut[:cut.index("\n}\n") + 2] + + picker_js = ("window.showSaveFilePicker = async () => ({});" + if picker else "") + script = tmp_path / f"{name}.mjs" + script.write_text(f""" +const out = []; +let live = 0, peak = 0; +const asked = []; +// Stands in for _openDownloadTarget: records how many are open at once, and +// whether each was told it is not the first of its batch. +const _openDownloadTarget = async (name, size, opts, swSize, flags) => {{ + live += 1; peak = Math.max(peak, live); + asked.push(!!(flags && flags.batched)); + if (name === 'stuck') return await new Promise(() => {{}}); + await new Promise(r => setTimeout(r, 20)); + live -= 1; + if (name === 'boom') throw new Error('refused'); + return {{ name }}; +}}; +// Only a browser with a Save As dialog has anything to serialise. +globalThis.window = {{}}; +{picker_js} +let _targetQueue = Promise.resolve(); +let _targetsInFlight = 0; +const TARGET_QUEUE_BUDGET_MS = {budget_ms}; +""" + lift("function _openTargetInTurn") + lift("function _waitBriefly") + f""" +{body} +console.log(JSON.stringify(out)); +""") + proc = subprocess.run(["node", str(script)], capture_output=True, text=True) + assert proc.returncode == 0, proc.stderr + return json.loads(proc.stdout) + + def test_targets_are_opened_one_at_a_time(tmp_path): """ A browser shows one file picker at a time and grants one per user gesture, @@ -378,8 +424,7 @@ def test_targets_are_opened_one_at_a_time(tmp_path): Opening the target inside `prepare` — so the row appears at the click rather than tens of seconds later — removed the accident, and four pickers raced. Reported from Chrome: one file downloaded, a prompt for the second, the - other two timed out. Firefox and Electron never noticed, because neither - opens a picker at all, which is why this reached one browser only. + other two timed out. The queue is on the *targets*, never on the rows: every download still appears the moment it is asked for. @@ -387,43 +432,60 @@ def test_targets_are_opened_one_at_a_time(tmp_path): Queueing alone was not enough: a second dialog with no gesture behind it still waits for a human, and the two behind it wait for the dialog. So everything that has to wait its turn is also marked `batched`, which the - opener reads as "do not ask" — see the streamed-path branch below. + opener reads as "do not ask" — see the streamed-path branch in + test_memory_ceiling.py. """ - src = (STATIC / "file-utils.js").read_text() - fn = src[src.index("function _openTargetInTurn"):] - fn = fn[:fn.index("\n}\n") + 2] - - script = tmp_path / "case.mjs" - script.write_text(""" -const out = []; -let live = 0, peak = 0; -// Stands in for _openDownloadTarget: records how many are open at once. -const asked = []; -const _openDownloadTarget = async (name, size, opts, swSize, flags) => { - live += 1; peak = Math.max(peak, live); - asked.push(flags && flags.batched); - await new Promise(r => setTimeout(r, 20)); - live -= 1; - if (name === 'boom') throw new Error('refused'); - return { name }; -}; -let _targetQueue = Promise.resolve(); -let _targetsInFlight = 0; -""" + fn + """ + peak, statuses, batched = _turn_harness(tmp_path, "one_at_a_time", """ const results = await Promise.allSettled( ['a', 'boom', 'c', 'd'].map(n => _openTargetInTurn(n))); out.push(peak); out.push(results.map(r => r.status).join(',')); out.push(asked); -console.log(JSON.stringify(out)); """) - proc = subprocess.run(["node", str(script)], capture_output=True, text=True) - assert proc.returncode == 0, proc.stderr - peak, statuses, batched = json.loads(proc.stdout) assert peak == 1, f"{peak} targets were being opened at once" - assert batched == [False, True, True, True], ( - "only the first of a batch holds the user's gesture; the rest must be " - "opened without asking") # And one refusal must not stop the rest: a chain that breaks on a rejection # leaves every later download unable to open anything at all. assert statuses == "fulfilled,rejected,fulfilled,fulfilled" + assert batched == [False, True, True, True], ( + "only the first of a batch holds the user's gesture; the rest must be " + "opened without asking") + + +def test_a_browser_with_no_dialog_does_not_queue_at_all(tmp_path): + """Firefox and Safari have no `showSaveFilePicker`, so no two openings there + can race a dialog and there is nothing for a queue to protect. + + Queueing them anyway was a regression: four downloads that had always opened + their targets at the same time began waiting on the slowest, and all four + sat at "preparing". A queue that buys nothing must not be paid for. + """ + peak, = _turn_harness(tmp_path, "no_picker", """ +await Promise.all(['a', 'b', 'c', 'd'].map(n => _openTargetInTurn(n))); +out.push(peak); +""", picker=False) + assert peak == 4, ( + f"only {peak} target opening(s) ran at once; without a dialog to " + "serialise, all four must proceed together as they did before") + + +def test_one_stuck_opening_does_not_hold_the_others_for_ever(tmp_path): + """`_targetQueue` is never reset, so an opening that never settles would + otherwise leave the page unable to start any download again — a panel that + only a reload can fix. + + The budget is 60 ms here; in the page it is ninety seconds, long enough that + a real dialog is never cut in front of. + """ + statuses, batched = _turn_harness(tmp_path, "stuck", """ +const first = _openTargetInTurn('stuck'); +first.catch(() => {}); +const rest = await Promise.allSettled( + ['b', 'c'].map(n => _openTargetInTurn(n))); +out.push(rest.map(r => r.status).join(',')); +out.push(asked); +""", budget_ms=60) + assert statuses == "fulfilled,fulfilled", ( + "an opening that never settles must not strand the ones behind it") + assert batched == [False, True, True], ( + "the stuck one is still the only holder of the gesture, so the released " + "openings must not try for a dialog of their own") |