[Web] Fix RPC argument parsing for new FFI string/bytes types#18683
[Web] Fix RPC argument parsing for new FFI string/bytes types#18683junrushao merged 1 commit intoapache:mainfrom
Conversation
Summary of ChangesHello @guan404ming, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical compatibility issue in the RPC server by enabling it to correctly handle recently introduced FFI string and bytes data types. By adding specific parsing logic for these new types, it ensures seamless communication when Python clients send string or byte arguments via RPC, thereby preventing runtime errors and maintaining system integrity. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where the RPC server was not correctly handling new FFI string/bytes types (kTVMFFIStr and kTVMFFIBytes) introduced in a recent FFI refactoring. The fix involves adding handlers to read and discard the duplicate type_index written by WriteFFIAny in C++ for these types. The changes are straightforward and seem to resolve the reported issue.
There was a problem hiding this comment.
I've confirmed that the C++ side does indeed write the type_index twice for kTVMFFIStr and kTVMFFIBytes. The flow is:
SendPackedSeq(inrpc_reference.h:314) writestype_indexfor each argument- For
kTVMFFIStr/kTVMFFIBytes, it falls through to thedefaultcase which callsWriteFFIAny WriteFFIAny(inrpc_endpoint.cc:237-243) writes the type_index again before the size and data
So the wire format for these types is: type_index(u32) | type_index(u32) | size(u64) | data The TypeScript fix correctly handles this by reading and discarding the duplicate type_index.
ab55e56 to
b8eab22
Compare
|
cc @junrushao |
Why
Recent FFI refactoring added new type indices kTVMFFIStr (65) and kTVMFFIBytes (66). The RPC server didn't
handle these, causing "cannot support type index 65/66" errors when Python sends string/bytes arguments via
RPC.
How
WriteFFIAnyin C++ writes thetype_indextwice for these types. The fix adds handlers that read and discard the duplicate type_index before reading the actual data.