Skip to content

fix: send ArrayBuffer request bodies safely - #139

Closed
PradyumnaShome wants to merge 1 commit into
margelo:mainfrom
PradyumnaShome:fix/arraybuffer-request-bodies-safe-lifetime
Closed

fix: send ArrayBuffer request bodies safely#139
PradyumnaShome wants to merge 1 commit into
margelo:mainfrom
PradyumnaShome:fix/arraybuffer-request-bodies-safe-lifetime

Conversation

@PradyumnaShome

Copy link
Copy Markdown

Summary

  • Send binary request bodies through Nitro as ArrayBuffer, without a base64 round trip, in both normal JS and worklet request construction.
  • Regenerate the Nitrogen C++/Swift/Kotlin bindings and upload native Data/ByteArray payloads on iOS and Android.
  • Cover ArrayBuffer, Uint8Array subviews, empty bodies, non-ASCII bytes, and unchanged string behavior.

Why

bodyBytes is currently typed as a string placeholder, and the request path does not upload the actual binary payload. A base64 bridge can restore correctness temporarily, but it adds encoding/decoding work and memory overhead. This PR carries the bytes as an ArrayBuffer instead.

Credit and relationship to existing work

Builds on and credits #112. The original commit remains authored by its contributor, and this draft intentionally preserves that approach rather than presenting it as unrelated work.

This is a separate PR because it rebases the change onto current main and adds the iOS lifetime fix described below. It is not intended as silent competition with #112; maintainers are welcome to choose the preferred PR or merge the safety work into the original approach.

#138 is a temporary correctness/base64 fallback. The two PRs are alternatives/stages, not changes that should both remain in the final implementation long-term.

iOS ArrayBuffer lifetime

Local native validation exposed a crash with the runtime diagnostic:

data() can only be accessed synchronously on the JS Thread... copy it first

The JSArrayBuffer passed into a Hybrid method is borrowed and must not survive the synchronous JS call boundary. Calling ArrayBufferHolder.toData(copyIfNeeded:) later from a Swift Task or detached work accesses it from the wrong thread.

request, requestSync, and prefetch now synchronously materialize the body as native Data before creating any Task. The request passed across the async boundary is rebuilt without the borrowed ArrayBuffer, and only native-safe values plus the copied Data are captured. Android similarly copies into a ByteArray before the Cronet request starts.

Compatibility and limitations

  • This uses native Data/ByteArray copies; it is intentionally safe, not zero-copy.
  • The generated native ABI changes from string to ArrayBuffer, so apps must perform a native rebuild after upgrading. This should be called out in release notes; callers using the old placeholder string form must migrate.
  • String bodies and FormData keep their existing paths.
  • Runtime prefetch() supports binary bodies with the same synchronous lifetime copy. Persisted app-start prefetch configuration does not serialize a borrowed ArrayBuffer; legacy string bodyBytes entries are no longer forwarded as binary data.

Validation

  • bun install
  • bun run nitrogen
  • bun --cwd packages/react-native-nitro-fetch test --runInBand — 15 passed, 2 existing todos
  • bun run typecheck
  • pre-commit lint and types hooks
  • ESLint and Prettier checks for the new request-body test

A clean native app rebuild and benchmark rerun are still pending. The iOS lifetime fix is based on the local native crash report above; no native benchmark numbers are claimed here.

@vercel

vercel Bot commented Jul 11, 2026

Copy link
Copy Markdown

@maxvaljan is attempting to deploy a commit to the Margelo Team on Vercel.

A member of the Team first needs to authorize it.

@PradyumnaShome
PradyumnaShome marked this pull request as ready for review July 11, 2026 06:41
@riteshshukla04

Copy link
Copy Markdown
Collaborator

Thanks for the PR . But this isnt the best way to do it . We should Ideally use Nitro Array buffers and something that works with prefetching too

@riteshshukla04

Copy link
Copy Markdown
Collaborator

Fixed in #140

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants