Repository navigation
fix(🏞️): log a worker job that throws instead of ending the app - #4346
Open
YevheniiKotyrlo wants to merge 5 commits into
Open
YevheniiKotyrlo wants to merge 5 commits into
YevheniiKotyrlo wants to merge 5 commits into
Conversation
RNSkWorker ran each job with nothing around it on a detached thread, so an exception a job let out left the thread and std::terminate ended the app. The worker now logs it, with what() for a std::exception, and runs the next job. WorkerTest checks the jobs behind a std::exception and behind anything else. RNSkLog.h, which the worker logs through, includes <cstdarg> and <cstdio> for what it uses, so the test can build it with nothing included before it, and run.sh adds React Native's JSI headers, which it includes.
YevheniiKotyrlo
marked this pull request as ready for review
October 28, 2026 13:32
YevheniiKotyrlo
added a commit
to YevheniiKotyrlo/react-native-skia
that referenced
this pull request
Oct 28, 2026
…release to their own PRs A worker job that throws is wcandillon#4346's, the idle trim owed after a busy one wcandillon#4347's, and the GPU memory Dawn keeps wcandillon#4352's: this PR drops their files, tests and hunks, renderPool()'s doc with them. cpp-tests/run.sh runs every host test again, as on main.
YevheniiKotyrlo
added a commit
to YevheniiKotyrlo/react-native-skia
that referenced
this pull request
Oct 28, 2026
The worker presents static canvases and makes the presented bitmaps, whose calls into Java can throw: this PR needs wcandillon#4346's catch, so it stacks on wcandillon#4346 as it does on wcandillon#4324 and wcandillon#4349.
wcandillon
approved these changes
Oct 29, 2026
wcandillon
self-requested a review
October 29, 2026 05:19
wcandillon
requested changes
Oct 29, 2026
wcandillon
left a comment
Owner
There was a problem hiding this comment.
This is a good catch. Now that we don't crash on throws, the caller (JsiSkImage), needs to reject the promise right?
YevheniiKotyrlo
marked this pull request as draft
October 29, 2026 10:42
A worker job that threw was logged and dropped, so makeRasterImage()'s promise stayed pending when the decode threw. RNSkWorker::post(make, deliver) runs make inside the catch every job runs in, then hands deliver what it returned, or null when it threw, and makeRasterImage() posts its decode that way: the promise rejects as it does for a decode that returns null. WorkerTest checks that a result is delivered once, and null once when make throws a std::exception or anything else. makeRasterImage()'s doc says it rejects when the readback or the decode fails.
Contributor
Author
|
Right: a47ce50 gives the worker |
YevheniiKotyrlo
marked this pull request as ready for review
October 29, 2026 12:40
YevheniiKotyrlo
added a commit
to YevheniiKotyrlo/react-native-skia
that referenced
this pull request
Oct 29, 2026
wcandillon#4346 now rejects makeRasterImage()'s promise when its decode throws, through RNSkWorker::post(make, deliver). The branch takes its head as it stands.
YevheniiKotyrlo
marked this pull request as draft
October 30, 2026 13:29
…b throws The encodes of encodeToBytes() and encodeToBase64(), and readPixels()' CPU read and its GPU read's conversion, run on the worker as jobs that complete their promise from inside, so a job that threw was logged and its promise stayed pending, and on a worklet runtime its tick kept re-arming. They post through RNSkWorker::post(make, deliver), as makeRasterImage()'s decode does: make returns the encoded data or the pixels read, null when there is none, and deliver completes with it, so a job that throws settles as one that fails without throwing: the encodes reject and readPixels() resolves to null.
YevheniiKotyrlo
marked this pull request as ready for review
October 30, 2026 14:24
Contributor
Author
|
Updated to the latest |
YevheniiKotyrlo
added a commit
to YevheniiKotyrlo/react-native-skia
that referenced
this pull request
Oct 30, 2026
main makes readbacks asynchronous (wcandillon#4368): readPixels() and the encodes return promises, and DawnContext follows each readback until its callback runs. The branch's readPixels() into a caller's SkImageInfo, which the image presenter reads its frames back through, takes that tracking. SkiaImageView on the web, the branch's e2e readers in setup.tsx and the specs that read through them, ClippedSubviews' own reader among them, await the pixels. JsiSkImage.h and Image.ts are wcandillon#4346's merge of the same main (7dfd058).
YevheniiKotyrlo
added a commit
to YevheniiKotyrlo/react-native-skia
that referenced
this pull request
Oct 30, 2026
wcandillon#4346 now settles the promises of the encodes and of readPixels() when their job throws, through RNSkWorker::post(make, deliver), on main's asynchronous readbacks. The branch takes its head as it stands.
This branch has not been deployed
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.
Problem
RNSkWorker::run()calls each job with nothing around it, on the thread the worker detaches:job = std::move(_jobs.front()); _jobs.pop_front(); } job();An exception a job lets out leaves the thread's function, and the standard then calls
std::terminate: the app ends. Onmainthe worker runs the encodes ofencodeToBytes()andencodeToBase64(),readPixels()' read of a CPU image and its conversion of a GPU readback, andmakeRasterImage()'s decode of an encoded image, each of which throws only when an allocation fails; any job posted to the worker later is in the same position.Solution
run()catches what a job throws, logs it throughRNSkLogger::logToConsole, withwhat()for astd::exceptionasRNSkGraphiteProducerlogs a failed replay, and runs the next job. A throw now fails its own job, not the app. The format overload writes into a fixed-size buffer, so the handler allocates nothing and logs astd::bad_alloctoo.Each of those jobs completes its promise from inside the job, so with the catch alone a job that throws would leave its promise pending, and on a worklet runtime the scheduler's tick re-arming. A job with a result goes through
post(make, deliver)instead: the worker runsmakeinside that catch and handsdeliverwhat it returned, or null when it threw, asDawnContext::readPixels()hands its callback null for a failed read. The four jobs post that way, eachdelivercompleting the promise, so a job that throws settles its promise as one that fails without throwing does: the encodes andmakeRasterImage()reject, andreadPixels()resolves to null. The settling reaches the runtime as every result does; on the React Native runtime that goes through React Native'snoexceptinvokeAsync, which allocates the task it schedules: a heap too exhausted for that ends the app there.run.shadds React Native's JSI headers (JSI_INCLUDE), whichRNSkLog.hincludes.Tests
cpp-tests/WorkerTest.cppchecks that the job behind one that throws runs, and thatpost(make, deliver)delivers once, null whenmakethrew. Its first case aborts onmain's worker, and withdeliver(make())as the result job, which is how the jobs complete their promise onmain, the result case fails:With the fix,
yarn test-cppprintsWorkerTest: passed.Verification
cpplint with CI's flags clean ·
yarn test-cpp2 of 2 passed (g++ 15.2) ·WorkerTestpassed with Apple clang 21 (Xcode 27.1,-Wall -Wextra, no warning), andJsiSkImage.hcompiles with it against the macOS and iOS SDKs ·yarn lintandyarn tscclean · the example's Android release build (arm64-v8a) succeeds, with no warning inpackages/skia· on a Galaxy S23 (Android 14), theReadback,ImageEncoding,DataEncodingandImagee2e specs pass, and with each of the four jobs made to throw, every promise settles as above, where withmain's jobs beside the catch each stays pendingNot run here: the example's iOS and macOS builds (CI's
build-test-ios,build-test-macosandbuild-test-ios-spm).No existing issue or PR covers this: I searched for
RNSkWorker,worker thread abort,uncaught exception threadandterminate called.Related
#4328 stacks on this PR: its static canvases present on this worker, and on Android its presented bitmaps are made there through calls into Java.
Base
mainat3184116c9, merged in by7dfd0589b.