Resolve typed-RPC promises with server error envelopes (and clean up collatedPromises)#55
Open
jack-bot-slims wants to merge 1 commit into
Open
Conversation
…collatedPromises) When a server emits an `Envelope.error` with a request `cid`, the receive loop only attempted to cast the matching promise to `EventLoopPromise<Any>` or `EventLoopPromise<Google_Protobuf_Empty>`. Typed RPC promises (registered as `EventLoopPromise<Nakama_Api_Rpc>`, `Nakama_Realtime_Channel`, etc.) matched neither, so the error was silently dropped and `await promise.futureResult.get()` hung forever. Affected callers include `rpc`, `joinChat`, `createMatch`, `addMatchmaker`, `followUsers`, `createParty`, `addMatchmakerParty`, and every other typed RPC. Void-returning RPCs accidentally worked via the `Empty` cast. Fix: extend the `.error` branch with explicit casts for every concrete promise type registered by `send<T>(env:)`, falling back to a logged error if a future type is added without updating this branch. Bonus: replace the `collatedPromises[response.cid]` lookup with `removeValue(forKey:)` so the promise is dropped from the dictionary in both success and error paths. Previously entries were never cleaned up — a slow leak in long-running sessions. Mirrors `delete this.cIds[message.cid]` in nakama-js. Co-Authored-By: Hendrik Schäfer <hendrik@musa.guide>
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.
Bug
When a server-side RPC throws and Nakama emits an
Envelope.errorcarrying the requestcid, the receive loop inSources/Nakama/Socket.swiftonly attempts two casts on the matchingcollatedPromises[cid]:Typed-RPC promises are registered by the generic
send<T: Message>(env:)asEventLoopPromise<T>for the concreteT(e.g.Nakama_Api_Rpc,Nakama_Realtime_Channel, …). Neither cast matches, so the error is silently dropped andtry await promise.futureResult.get()blocks indefinitely.This affects every typed RPC path —
rpc,joinChat,createMatch,addMatchmaker,followUsers,createParty,addMatchmakerParty,listPartyJoinRequests, etc. Void-returning RPCs accidentally work via theGoogle_Protobuf_Emptycast.Real-world reproduction
A server-side SQL bug made one of our RPCs return an error envelope in ~2 ms. The Swift client never observed any response — the awaiting task hung. Downstream we held a request-serialising mutex, which then stayed locked until heartbeat-driven force-release fired ~26 s later. After patching this branch locally, the same server error surfaces immediately as the expected
NakamaRealtimeError.Wire-protocol confirmation
Server-side
pipeline_rpc.gosetsCid: envelope.Cidon error envelopes (ProtoEnvelope.cidisstring cid = 1, populated on all responses). No protocol ambiguity — purely an SDK-side gap.Fix
Extend the
.errorbranch with explicit casts for every concrete promise type registered bysend<T>(env:):Nakama_Api_RpcNakama_Realtime_ChannelNakama_Realtime_ChannelMessageAckNakama_Realtime_MatchNakama_Realtime_MatchmakerTicketNakama_Realtime_StatusNakama_Realtime_PartyNakama_Realtime_PartyMatchmakerTicket…in addition to the existing
AnyandGoogle_Protobuf_Emptycasts. If a future return type is added toSocket.swiftwithout updating this branch, an error is now logged instead of the failure being silently swallowed.Bonus: clean up
collatedPromisesThe same code path previously never removed entries from
collatedPromises— neither on success nor on error. In long-running socket sessions this slowly leaks memory. Switched the lookup toremoveValue(forKey:)so the entry is dropped before dispatching to the appropriate promise. Mirrorsdelete this.cIds[message.cid]in nakama-js.Notes for reviewers
as!force-casts and adefaultbranch that falls back toEmpty.succeed. Left both untouched to keep the diff minimal and focused on the error path. Happy to align them in a follow-up if preferred.collatedPromisesto store an erased(rawPromise, failer: (Error) -> Void)tuple to avoid the cast list entirely. It is structurally cleaner but a bigger change; went with the additive cast list here for review safety. Glad to redo as the tuple approach if maintainers prefer.Test plan
NakamaRealtimeErrorinstead of hanging.collatedPromisesis empty after a sequence of successful + failing RPCs on a long-lived socket.