Repository navigation
Conversation
`URLSession:dataTask:didReceiveData:` parsed each network read on its own.
It looked for the first `{` and the next `}` within that read and ignored
everything else. The real-time stream is a JSON array of small config
update messages, and NSURLSession can deliver it in arbitrary pieces. So a
message split across reads was dropped, only the first of several messages
in one read was handled, and a read that finished one message and started
the next handled only the next one. A dropped template version message
meant no auto-fetch and no listener callback until a later message or the
next stream connection.
Buffer the stream data and handle every complete message, keeping a partial
message until the rest of it arrives, like the Android and Web SDKs do.
Clear the buffer when a new stream response starts, and stop handling
messages once the server disables real-time updates.
Fixes #16793
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 the 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 counterproductive. 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. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves Firebase Remote Config real-time updates by buffering incoming stream data in _pendingStreamData to ensure messages are not missed when split across network reads or received together in a single read. It introduces the nextConfigUpdateMessage helper to extract complete JSON objects from the buffer and includes comprehensive unit tests to verify these scenarios. Feedback suggests optimizing nextConfigUpdateMessage by using static NSData instances for the opening and closing braces to avoid redundant allocations.
| NSData *openingBrace = [NSData dataWithBytes:"{" length:1]; | ||
| NSData *closingBrace = [NSData dataWithBytes:"}" length:1]; |
There was a problem hiding this comment.
To avoid allocating and deallocating NSData objects for the opening and closing braces on every call to nextConfigUpdateMessage, you can use static variables initialized once with dispatch_once.
static NSData *openingBrace;
static NSData *closingBrace;
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
openingBrace = [NSData dataWithBytes:"{" length:1];
closingBrace = [NSData dataWithBytes:"}" length:1];
});There was a problem hiding this comment.
[paulb777-bot] Thanks. nextConfigUpdateMessage runs once per received message, plus once to find that no complete message is left, and messages on this stream are rare, since they announce template updates. Two 1-byte NSData objects per message aren't measurable next to the JSON parsing and the fetch that each message triggers, so I've kept the code without static state.
Problem
-[RCNConfigRealtime URLSession:dataTask:didReceiveData:]parsed each network read on its own. Itlooked for the first
{and the next}within that read, parsed only that substring, and ignoredthe rest.
The real-time stream is a JSON array of small config update objects (
[{"latestTemplateVersionNumber": "5"}, then,{...}), and NSURLSession can deliver it in arbitrary pieces. That caused threeproblems:
chunking reported in RCNConfigRealtime crash "CFString cannot be created from a negative number of bytes" #14094: reads like
},{"..."}.A dropped
latestTemplateVersionNumbermessage means no auto-fetch and no listener callback until alater message or the next stream connection. When the #14094 crash was fixed in #14518, buffering
was named as the long-term fix. The Android and Web SDKs already accumulate stream data until a
message is complete.
Change
_pendingStreamData), and every complete message init is handled (
nextConfigUpdateMessage).{(array punctuation, whitespace) is dropped.}arrives.didReceiveResponse:).data is ignored. This case couldn't come up before, because at most one message per read was
handled.
evaluateStreamResponse:error:are unchanged. Messages are still assumed tobe flat JSON objects, as before and as on Android/Web.
RCNRemoteConfigTest.m.Interplay with #16760, which also touches
didReceiveData::+JSONObjectWithData:with nil, which raises.[nil containsString:]is NO, and the buffered bytes are parsed instead of the string.read ever ended mid-character, so it can go when the two are combined.
testURLSessionDelegateHandlesChunkedJSON, so that's a trivial "keep both" conflict, plus theusual CHANGELOG one.
Testing
RCNRemoteConfigTestcan't run on its own, on main or here. ItssetUpcreates anRCNConfigDBManagerdirectly, butgIsNewDatabaseQueueis only created in+sharedInstance. Sodispatch_syncon a NULL queue segfaults unless an earlier test class, such asFIRRemoteConfigComponentTest, has already created the shared instance. Run the whole scheme, oradd
-only-testing:RemoteConfigUnit/FIRRemoteConfigComponentTest.testRealtimeStreamHandlesMessageSplitAcrossReadsevaluated nothing; expected[5].testRealtimeStreamHandlesSeveralMessagesInOneReadevaluated[5]; expected[5, 6].testRealtimeStreamHandlesReadsThatEndAndStartMessagesevaluated[6]; expected[5, 6, 7].testRealtimeStreamDropsPartialMessageWhenNewResponseStartsandtestRealtimeStreamStopsHandlingMessagesWhenRealtimeIsDisabledcover the buffer reset and thedisabled check. They pass without the fix, because only one message per read was handled.
(
<null>) instead of[6], andevaluateStreamResponse:error:called twice instead of once.Fixes #16793