feat(fcm): Improve HTTP/2 session error handling.#3126
feat(fcm): Improve HTTP/2 session error handling.#3126jonathanedey wants to merge 5 commits intoerror-revampfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the messaging service's HTTP/2 session management, replacing the specialized FirebaseMessagingSessionError with a more standard error aggregation approach. By utilizing AggregateError, the system now captures multiple session-level failures and includes them in the BatchResponse. The Http2SessionHandler was also updated to collect errors throughout the session lifecycle. Feedback focuses on a logic error in createSession where the internal session state is not correctly updated, a recommendation for null-safety when checking session status, and an improvement to error messaging to ensure session failure details are not lost during serialization.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the HTTP/2 session error handling in the messaging module, replacing the promise-rejection model in Http2SessionHandler with an error-collection mechanism. This change allows session errors to be aggregated into the BatchResponse using AggregateError, leading to the removal of the FirebaseMessagingSessionError class. The PR also includes various formatting and semicolon fixes for consistency. Feedback includes suggestions to simplify redundant logic in the error parsing function and to maintain consistency in the use of optional chaining when accessing error properties.
This PR refactors how FCM http2 session errors are handled making use of the new
causeattribute onFirebaseErrorsto allow failing of request at the stream level with context on session error cause.BatchResponseattached to the error to resolve, we now let streams fail naturally and attach session errors ascauseto failed messages inBatchResponse.AggregateErrorsupport when multiple session errors are captured during a batch.Http2SessionHandlerby removing the promise-based failure tracking (invoke()), since we no longer need to catch rejections insendEach().FirebaseMessagingSessionError