fix(core): resolve asset protocol requests off the webview thread (fix #7434) - #15836
Closed
PathGao wants to merge 3 commits into
Closed
fix(core): resolve asset protocol requests off the webview thread (fix #7434)#15836PathGao wants to merge 3 commits into
PathGao wants to merge 3 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #7434.
asset://requests are served by a handler registered through wry'swith_asynchronous_custom_protocol(crates/tauri-runtime-wry/src/lib.rs), an API whose point is that the handler may return immediately and respond later from another thread. The asset handler did not do that — it calledget_response, which is synchronousstd::fsI/O, inline and only then responded, so the whole read ran on the thread the webview invoked it on.That is the thread running the event loop, so a read that blocks stalls every window driven by that event loop, not just the one that asked. This matches #7434, where an image on an unreachable SMB path freezes the entire app, and the reporter's own guess that "the rust-end was blocked".
This wraps the existing handler body in
crate::async_runtime::spawn_blockingand changes nothing else.get_responseis untouched, so the range/206 path, theHEADpath, theSafePathBufand scope checks, the CORS header and the Android external-storage branch all keep their current behaviour — every one of them is reached through that single call, and bothresponder.respondarms moved with it, so there is no route that still responds from the calling thread.Why
spawn_blockingrather thanspawn#15220 did this for the sibling
tauri://protocol and usedcrate::async_runtime::spawn, because thatget_responseisasync. This one is not: since #15117 (refactor(tauri): use blocking apis where it makes sense) the asset body is plainstd::fs, which is what a blocking pool is for, andspawnwould park an async worker on a blocking read. Happy to switch tospawnif you would rather the two protocols read the same way.scopeisArc-backed so cloning it is a refcount bump, andwindow_originis a shortString. I kept it to those two clones rather than introducing anArc<Context>as #15220 did, to hold the diff to the handler.Testing
On macOS (aarch64):
cargo test -p tauri --all-features— 61 unit tests and 122 doc tests, all passingcargo clippy -p tauri --all-targets --all-features— cleancargo fmt --all -- --check— cleanI added one test,
does_not_block_the_calling_thread. It uses a FIFO as a stand-in for a path that never answers — opening one for reading blocks until a writer appears — and asserts that the handler has returned while nothing has been read yet. I checked that it actually exercises the defect: with this change reverted and the test kept, it blocks indefinitely instead of passing. It is#[cfg(all(test, unix))]and calls the handler on a spawned thread, so a regression fails the test rather than hanging CI.I separately checked, with a throwaway test I have not included, that the range path behaves identically through the new call: full
GET,bytes=10-19,bytes=0-,bytes=-100, an unsatisfiable range,HEAD, a missing file and a scope denial all produce the same status, the samecontent-range/content-length/accept-rangesheaders and the same bodies as before. I left it out to keep the diff to the fix; glad to add it if you want the coverage.What I could not verify
I have no SMB share or other unreachable network path available, so I have not reproduced #7434 itself, and I have not confirmed the fix against the reporter's scenario. The case that this fixes it rests on the code and on the FIFO test, not on the original repro.
One behaviour change worth flagging
On Android, wry blocks
shouldInterceptRequestwaiting on the responder withMAIN_PIPE_TIMEOUT * 3— 30s,src/android/mod.rsin wry 0.56. Today the asset handler responds inline, so that deadline is never reached; after this change a read taking longer than 30s would time out and the request would fall through to Android's network stack instead of eventually succeeding. #15220 already puttauri://in the same position. That seemed the right trade against freezing the app for those same 30s, but I am happy to handle it differently if you disagree.