[RNE Rewrite] feat: coded, worklet-safe error handling - #1354
Conversation
Proposal for the "Error Handling in TS" item of #1208. Keeps main's machine-readable `code` contract, adopts the rewrite's message discipline, and makes both survive the worklet boundary. - errors.config.ts: 46 codes -> 13. ExecuTorch's codes are no longer mirrored into the RNE enum; they travel in a separate `etCode` field so the two code spaces stay independent. Drops `Ok`, the UnknownError/Internal overlap, and the codes the rewrite's design made unreachable (`ModuleNotLoaded` has no analogue now that a task cannot exist unloaded). - RnExecutorchError sets `name`, keeps `instanceof` working through subclasses via `new.target`, and no longer shadows the native `cause`. - Worklets cannot carry class identity across runtimes, so `rnExecutorchError()` throws the plain-data form inside them and `wrapAsync` rebuilds a real error on the RN runtime. `isRnExecutorchError()` works in both, and is the recommended check. - Native: `CodedError` + `guarded()` at every host function, so a code raised deep in the native stack reaches the app's catch block. - Rewires `yarn codegen:errors` and the CI drift check, which were orphaned on this branch.
Follow-up to the error-handling proposal: closes the two gaps left open and updates the example apps. Native (items 1 + 2) - `error::guarded(...)` now wraps all 31 host function registrations, not just model.cpp's 3. 127 remaining `jsi::JSError` / `std::runtime_error` / `std::invalid_argument` throws across 13 files became `CodedError`. Nothing but `throwJs` raises an uncoded exception any more. - This also closes the sync-worklet hole: `guard` attaches `code` to the JS Error it builds, so a VisionCamera frame processor calling a task worklet directly now receives coded errors too. Previously only the `wrapAsync` path had a fallback, and it could only ever say `Internal`. - Renames `ModelBusy`/`ModelDisposed` to `ResourceBusy`/`ResourceDisposed`. Tensors and tokenizers have exactly the same two states as models, and they are where most of these throws live. Still 13 codes. Demo apps (item 4) - Adds `describeError` / `isBusyError` / `isDisposedError` per app, replacing five different ad-hoc idioms (`e.message || String(e)`, `String(loadError)`, `e?.message ?? String(e)`, ...) across 38 sites. - text-embeddings matched disposal with `/disposed/i.test(msg)` against the error *text*. Now a `ResourceDisposed` code check. - classification drops `ResourceBusy` instead of flashing an error, on both `classify` and `classifyWorklet` — the same code arrives over either boundary. `jsi::JSError` still passes through `guard` untouched, so an error thrown by an app's own callback (e.g. inside `tensor.through(fn)`) is never rewritten with our name and code.
- utils.cpp: the error include and using-declarations were sitting inside the `#elif defined(__APPLE__)` branch, so `CodedError` and `ErrorCode` were undeclared on every non-Apple target. Moved to file scope. This is why the macOS-only local syntax check did not catch it. - model.cpp, tensor.cpp: drop namespace aliases that misc-unused-alias-decls flagged. Inside rnexecutorch::core::*, unqualified `error` already resolves to the sibling rnexecutorch::core::error, and model.cpp's `jsi` alias lost its last user when the local unwrap templates were replaced by error::unwrapEt. The alias is kept in extensions::*, where sibling lookup does not reach it.
skills-maintenance requires skills to be updated in the same change that shifts an idiom. This was missed: add-native-extension still taught `throw jsi::JSError(rt, ...)` and a bare `createFromHostFunction(..., fnBody)`, which are now exactly the two patterns the convention forbids. - New .agents/skills/error-handling/SKILL.md: the two throw forms and why they differ (worklet runtimes cannot carry class identity), catching via isRnExecutorchError, the 13-code table, the test to apply before adding a code, the C++ CodedError/guarded rules, and the app describeError helpers. - add-native-extension: template now throws CodedError and registers through error::guarded, including the namespace-alias rule (needed in extensions::*, dead in core::*) and the file-scope include placement that broke Android. - add-task-pipeline, model-schema-validation: throw-with-a-code guidance plus checklist items. - verify-and-build: `yarn codegen:errors` step, and the two verification traps this branch hit (Homebrew clang-tidy reports checks CI lacks; a macOS-only syntax check cannot see platform-conditional code). - core-guidelines and README: index the new skill.
- conversions.h: drop the redundant `core::` qualification. The file is in rnexecutorch::core::conversions, so unqualified `error` already resolves to the sibling rnexecutorch::core::error. - error.h: mark both CodedError constructors explicit. - error.h: use std::format for the unwrapEt context prefix instead of string concatenation, matching the rest of the codebase.
barhanc
left a comment
There was a problem hiding this comment.
Only top level general comments for now as I want to first discuss the overall premises of the implementation before diving into details.
There was a problem hiding this comment.
I think we should leave the example apps as they were. They are mostly a testing ground for us and we should see the full low-level errors (e.g. even to test this new error handling). Streamlining apps is a scope of another issue #1288 and I'm even thinking if these user-facing example apps showcasing the library shouldn't actually be in a separate repository like react-native-executorch-examples which installs rn-executorch from npm, etc. (so it is a full app-dev experience instead of the slightly contrived one we have with the apps/ directory). Regardless, I think these apps should be mainly for us to test and all errors should be surfaced exactly.
There was a problem hiding this comment.
Agreed. I changed them mainly to test if the new flow works. But I like the idea of spliting examples into two categories and making it in one specialized PR.
There was a problem hiding this comment.
I really like the idea of using a guarded wrapper. There are however some rough edges in this implementation.
- First of all using
guardedwe should never throwjsi::JSErroranywhere in the C++ code therefore I don't see the purpose of thethrowJsfunction here. - I feel like to make it even clearer that now we should only use C++ exceptions the
makeJsshould be replaced with something likeinline void throwJsRnExecuTorchError(jsi::Runtime &rt, const RnExecuTorchException &e)and theguardshould look more like
try {
return std::forward<Fn>(fn)();
} catch (const RnExecuTorchException &e) {
errors::throwJsiRnExecuTorchError(rt, e);
} catch (const jsi::JSError &) {
// Already a JavaScript value, and possibly one thrown by user code
// called back into. Pass it through untouched.
throw;
} catch (const std::exception &e) {
errors::throwJsiRnExecuTorchError(rt, errors::RnExecuTorchException(errors::RnExecuTorchErrorCode::Unknown, e.what()));
} catch (...) {
errors::throwJsiRnExecuTorchError(rt, errors::RnExecuTorchException(errors::RnExecuTorchErrorCode::Unknown, "Unknown native exception occurred"))
}so there is no way of throwing the error to JS without constructing the C++ exception first.
- The naming could be a bit clearer. I would prefer
RnExecuTorchExceptioninstead ofCodedErroras the second one doesn't make it immediately clear that this is our custom error and the same forErrorCode. I would also renameetCodeto something more descriptive e.g.executorchRuntimeErrorCodeor some variation on that if it is too long.guardedis also a bit too generic for my liking, maybe something likewithJsiErrorHandling(fnBody)orjsiErrorGuarded(fnBody)would be better, but I won't push too hard on this one---error::guarded(fnBody)has a nice ring to it ;). - I would leave the
unwraps as a quick, dirty helper functions instead of putting them undererrorsnamespace. They should, however, only be available now with one signature without thertJSI runtime.
There was a problem hiding this comment.
I'm really against having the error codes auto-generated from a script. It feels like a solution that goes against the general way we implemented the TS-JSI interface up till now. Throughout the whole library we have clear mirroring of TS and C++ files that implement common interface which is shared implicitly between these two layers. We don't, for example, auto-generate dtypes, schema types, function signatures or host object interfaces from some spec/config file. The pattern is that TS contains the definitions and C++ JSI layer conforms to what TS has. Following this in error.{h, cpp} files would mean implementing the C++ equivalent of TS like so (simplifying a bit ofc)
enum class RnExecuTorchErrorCode {
LoadFailed,
ExecutionFailed,
SchemaMismatch,
InvalidArgument,
ResourceUnavailable,
Unknown
};
constexpr const char* errorCodeToString(RnExecuTorchErrorCode code) {
switch (code) {
case RnExecuTorchErrorCode::LoadFailed: return "LOAD_FAILED";
case RnExecuTorchErrorCode::ExecutionFailed: return "EXECUTION_FAILED";
case RnExecuTorchErrorCode::SchemaMismatch: return "SCHEMA_MISMATCH";
case RnExecuTorchErrorCode::InvalidArgument: return "INVALID_ARGUMENT";
case RnExecuTorchErrorCode::ResourceUnavailable: return "RESOURCE_UNAVAILABLE";
default: return "UNKNOWN";
}
}
class RnExecuTorchException : public std::runtime_error {
explicit RnExecuTorchException(RnExecuTorchErrorCode code, const std::string &message)
: std::runtime_error(message),
code_(code) {}
explicit RnExecuTorchException(ErrorCode code, const std::string &message, executorch::runtime::Error etError)
: std::runtime_error(message),
code_(code),
etRuntimeErrorCode_(static_cast<int32_t>(etError)) {}
RnExecuTorchErrorCode code_;
std::optional<int32_t> etRuntimeErrorCode_;
};
inline void throwJsiRnExecuTorchError(jsi::Runtime &rt, const RnExecuTorchException &e) {
auto errorCtor = rt.global().getPropertyAsFunction(rt, "Error");
auto errObj = errorCtor.call(rt, jsi::String::createFromUtf8(rt, e.what())).asObject(rt);
errObj.setProperty(rt, "name", jsi::String::createFromUtf8(rt, "RnExecuTorchError"));
errObj.setProperty(rt, "code", jsi::String::createFromUtf8(rt, errorCodeToString(e.code_)));
if (e.etRuntimeError_.has_value()) {
errObj.setProperty(rt, "etRuntimeErrorCode", *e.etRuntimeErrorCode_);
}
throw jsi::JSError(rt, std::move(errObj));
}There was a problem hiding this comment.
Indeed, this would be the only one codegened part in the repository. I took it from the old flow where errorcodes were also generated.
There was a problem hiding this comment.
Continuing the comment from error_codes.h, we should just have error.ts inside core/ that would define the error codes and the custom error class. I think using type union is more idiomatic than enum here, so I would rather see something like (screaming case or pascal case)
const VALID_ERROR_CODES = [
'LOAD_FAILED',
'EXECUTION_FAILED',
'SCHEMA_MISMATCH',
'INVALID_ARGUMENT',
'INVALID_STATE',
'RESOURCE_DISPOSED',
'RESOURCE_BUSY',
'DOWNLOAD_FAILED',
'DOWNLOAD_ABORTED',
'UNKNOWN',
] as const;
export type RnExecuTorchErrorCode = (typeof VALID_ERROR_CODES)[number];Also I believe some of the error codes are too specific (e.g. the tokenizer or not supported errors). Imo the specific errors should only be for cases where the user genuinely can take some action upon fail to recover (good examples are download errors or resource errors). Other errors can just be general categories for tools like Sentry to group them (e.g. execution or load errors are generally non-recoverable so subdividing them doesn't provide anything on top of the error message). But this can be discussed later as these are details.
There was a problem hiding this comment.
Regarding nunber of error codes I had a dillema. Now there are 13 of them, but indeed I though to make this group much smaller, e.g. 5. And your argumentation about being too specific without real gains works for me. Regarding type union vs enum let's move with TU then.
There was a problem hiding this comment.
This design with both class RnExecuTorchError and rnExecuTorchError() is flaky. Since we cannot really use classes due to how worklets work, I think we must do something like this
export type RnExecuTorchError<C extends RnExecuTorchErrorCode = RnExecuTorchErrorCode> = Error & {
name: 'RnExecuTorchError';
code: C;
etRuntimeErrorCode?: number;
};
export function RnExecuTorchError<C extends RnExecuTorchErrorCode>(
code: C,
message: string,
etRuntimeErrorCode?: number
): RnExecuTorchError<C> {
'worklet';
const err = new Error(message) as RnExecuTorchError<C>;
err.name = 'RnExecuTorchError';
err.code = code;
if (etRuntimeErrorCode !== undefined) {
err.etRuntimeErrorCode = etRuntimeErrorCode;
}
return err;
}
export function isRnExecuTorchError<C extends RnExecuTorchErrorCode = RnExecuTorchErrorCode>(
err: unknown,
code?: C
): err is RnExecuTorchError<C> {
'worklet';
if (err === null || typeof err !== 'object') {
return false;
}
if (!('name' in err) || err.name !== 'RnExecuTorchError') {
return false;
}
if (!('code' in err) || typeof err.code !== 'string') {
return false;
}
if (!(VALID_ERROR_CODES as readonly string[]).includes(err.code)) {
return false;
}
return code === undefined || err.code === code;
}and in runtime.ts
export function wrapAsync<Args extends any[], R>(
fn: (...args: Args) => R,
runtime: WorkletRuntime = defaultWorkletRuntime
) {
return async (...args: Args): Promise<R> => {
const result = await runOnRuntimeAsync(
runtime,
(argsArray) => {
'worklet';
try {
return { ok: true, value: fn(...argsArray) };
} catch (e: any) {
let error;
if (isRnExecuTorchError(e)) {
error = {
name: 'RnExecuTorchError',
code: e.code,
message: e.message,
etRuntimeErrorCode: e.etRuntimeErrorCode,
};
} else {
error = e?.message ?? String(e);
}
return { ok: false, error };
}
},
args
);
if (!result.ok) {
if (isRnExecuTorchError(result.error)) {
throw RnExecuTorchError(
result.error.code,
result.error.message,
result.error.etRuntimeErrorCode
);
}
throw new Error(result.error);
}
return result.value!;
};
}This way we can just throw throw RnExecuTorchError(code, message) everywhere.
There was a problem hiding this comment.
Yeah, I remembered that in worklets, we cannot use regular Errors and asked clanker to propose some solution that bypass this, but I agree that your solution is better
|
Agreed on all changes; I'll implement them after the weekend ;) |
Description
This branch had no error module: everything threw a plain
Errorwith a message, andwrapAsyncflattened anything down toe?.message.mainhas a code-based contract, but that contract cannot survive in worklets, etc.This keeps the machine-readable
codecontract, adopts this branch's message discipline, and makes both survive worklets and JSI.etCodefield so RNE and ET errors stay independent. DropsOk, theUnknownError/Internaloverlap, andModuleNotLoaded(not applicable anymore).RnExecutorchErrorsetsname, keepsinstanceofworking through subclasses vianew.target, nocauseanymore.rnExecutorchError()throws a plain data inside them andwrapAsyncrebuilds the error.isRnExecutorchError()works in both and is the recommended check.CodedErrorplusguarded()on all host functions, and rawjsi::JSError/std::runtime_error/std::invalid_argumentthrows converted, so a code raised deep in the native stack reaches the app's catch block. This also covers the sync worklet path used by VisionCamera frame processors, which previously had no normalization at all.Introduces a breaking change?
Type of change
Tested on
Testing instructions
yarn install && yarn workspace react-native-executorch prepareyarn typecheck && yarn lint, both clean.yarn codegen:errorsthengit diff --exit-codeonsrc/errors/codes.tsandcpp/core/error_codes.h, no output.Screenshots
Related issues
#1208
Checklist
Additional notes
jsi::JSErrordeliberately passes throughguarduntouched, so an error thrown by an app's own callback is never rewritten with our name and code.