Support for large RPC messages using data streams#1832
Support for large RPC messages using data streams#1832
Conversation
🦋 Changeset detectedLatest commit: f69d8a0 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
1egoman
force-pushed
the
rpc-with-compression-data-streams
branch
from
6128790 to
bc5f66d
Compare
1egoman
changed the title
1egoman
requested review from
ladvoc,
lukasIO,
pblazej,
stephen-derosa and
xianshijing-lk
1egoman
force-pushed
the
rpc-with-compression-data-streams
branch
from
fa9fde9 to
b6c177e
Compare
|
(rebased on top of latest |
1egoman
force-pushed
the
rpc-with-compression-data-streams
branch
from
e627f9b to
595a20d
Compare
| let rpcClientManager: RpcClientManager; | ||
| let mockStreamTextWriter: { | ||
| write: ReturnType<typeof vi.fn>; | ||
| close: ReturnType<typeof vi.fn>; | ||
| }; | ||
| let mockOutgoingDataStreamManager: OutgoingDataStreamManager; | ||
|
|
||
| beforeEach(() => { | ||
| mockStreamTextWriter = { | ||
| write: vi.fn().mockResolvedValue(undefined), | ||
| close: vi.fn().mockResolvedValue(undefined), | ||
| }; | ||
| mockOutgoingDataStreamManager = { | ||
| streamText: vi.fn().mockResolvedValue(mockStreamTextWriter), | ||
| } as unknown as OutgoingDataStreamManager; | ||
|
|
||
| rpcClientManager = new RpcClientManager( | ||
| log, | ||
| mockOutgoingDataStreamManager, | ||
| (_identity) => CLIENT_PROTOCOL_DATA_STREAM_RPC, // (other participant is "v2") | ||
| () => undefined, | ||
| ); |
There was a problem hiding this comment.
question: I still am not too pleased with how tightly coupled all these manager pieces are here. I think getting RtcEngine out of OutgoingDataStreamManager is probably fairly easy and worthwhile (though I think I'd prefer to do that in a follow up change) but even if so, RpcClientManager and OutgoingDataStreamManager still are quite coupled and it would be awkward to decouple them...
I thought that a pattern closer to how data tracks works would be worthwhile and while that was definitely helpful, it really only works for "fire and forget" type events like sending a data packet. It doesn't work all that well for request response type interactions (ie, calling an async function where you want to block until the function is complete, not just fire and forget, like calling outgoingDataTrackManager.streamText).
I think what I've got here works but I think it can get better. If others have ideas on how to do that, I'd be interested in hearing them!
| # RPC v2 Specification | ||
|
|
||
| ## Overview | ||
|
|
There was a problem hiding this comment.
Note to other reviewers: this spec was partially LLM generated and the thought is will act as a plan for LLMs to implement these same features in other sdks.
This will NOT be merged with this change, but I wanted a place for it to live that was not notion / etc since it's fairly tied to this code and should evolve along with it if review comments result in protocol changes.
There was a problem hiding this comment.
I think this is a good idea.
I do wonder if we should setup a repo to save those spec.mds, then we will use submodule to share it across sdks
There was a problem hiding this comment.
@xianshijing-lk @lukasIO @reenboog @ladvoc @MaxHeimbrock @pblazej
I am tagging a larger group of people in the hopes of getting this RPC v2 specification document reviewed. The web implementation is largely done at this point. Feel free to leave comments that would be relevant to the respective SDKs you work on, and then this document can serve as a prompt for a LLM to port this over to each respective SDK.
1egoman
changed the title
This is so that this client protocol value can be used to know what version of RPC that a remote client supports.
…rotocol "0" for now though
…ed in memory at once
…kas did in the original web example Register a data stream with an attribute, and listen for data streams with that attribute on the other end.
This could exist if the room is disconnected halfway through responding to a rpc request.
…licitly "version 2")
…ath to verify this fails
1. Break out request / responses onto seperate topics 2. Get rid of distinct RPC_RESPONSE_ID_ATTR
1egoman
force-pushed
the
rpc-with-compression-data-streams
branch
from
f58a747 to
f2b194f
Compare
|
(rebased on top of latest |
1egoman
requested review from
MaxHeimbrock,
ladvoc,
lukasIO,
pblazej and
reenboog
3 participants
Currently this prototype does the below:
RPCRequestpath when the advertisedclientProtocolis less than 1.RPCRequest/RPCResponsemessages.Also undertakes some signifigant refactoring to move RPC logic out into a
RpcClientManager/RpcServerManager, which given the increase in complexity makes the code a little less fragmented / easier to reason about.Todo
examples/rpc-benchmark- this is what is leading to the large diff size.Before potentially merging this, think about if it makes sense to keep this checked in or not. And if so, go through and update some of the docs to be a little more modern / correct (in particular, it still refers to the "legacy path < 1kb" stuff).I decided to check it out of version control for now given the goal of this has shifted to worry less about performance in the short term.