feat(fcm): Add HTTP2 support for `sendEach()` and `sendEachForMulticast()` #2550

jonathanedey · 2024-05-09T15:11:55Z

Added:

HTTP/2 support for sendEach() and sendEachForMulitcast()
New deprecated method enableLegacyTransport()

When sending messages using sendEach() or sendEachForMulitcast() a HTTP/2 connection is now used by default.

In order to use the legacy HTTP/1.1 versions of these methods, the enableLegacyTransport() method must be used. This method is already marked as deprecated and will be removed once the HTTP/2 transport is considered fully stable.

Related: #2488

…requests

…ntegration tests

test/unit/utils/api-request.spec.ts

lahirumaramba

Thanks, Jonathan!
The first pass (to quickly unblock you) LGTM. I will take another look at the tests.
Let's get an internal API proposal going soon

lahirumaramba · 2024-06-11T15:09:18Z

Thanks Jonathan! Let's make the changes we discussed in the API proposal and do another pass. We should be good to go then!

lahirumaramba · 2024-06-19T16:43:59Z

src/messaging/messaging.ts

+   * ```
+   * 
+   * @deprecated This is to be removed once the HTTP/2 transport is universally safe.
+   */


Can we get a TW review on this?

lahirumaramba · 2024-06-19T16:45:01Z

src/messaging/messaging.ts

+   * messaging.sendEach(messages);
+   * ```
+   * 
+   * @deprecated This is to be removed once the HTTP/2 transport is universally safe.


This is to be removed once the HTTP/2 transport implementation is universally safe. ?

Yeah, I'm not crazy about that wording either. Can we give a more concrete criteria than safety?

egilmorez

LG! But I do have comments :)

Can you use your eagle eye and do a scrub for literals that lack backticks Lahiru? Thanks!

egilmorez · 2024-06-25T17:59:47Z

src/messaging/messaging.ts

@@ -221,6 +223,22 @@ export class Messaging {
    return this.appInternal;
  }

+  /**
+   * Enables the use of the legacy HTTP/1.1 transport for sendEach() and sendEachForMulticast().


Should this be "layer," as in "the x transport layer?"

Let's do either that or drop the "the," with backticks like:

"Enables the use of legacy HTTP/1.1 transport for sendEach() and sendEachForMulticast().

egilmorez · 2024-06-25T18:00:30Z

src/messaging/messaging.ts

+   * messaging.sendEach(messages);
+   * ```
+   * 
+   * @deprecated This is to be removed once the HTTP/2 transport is universally safe.


Yeah, I'm not crazy about that wording either. Can we give a more concrete criteria than safety?

egilmorez · 2024-06-25T18:02:56Z

src/utils/api-request.ts

- * Default retry configuration for HTTP requests. Retries up to 4 times on connection reset and timeout errors
- * as well as HTTP 503 errors. Exposed as a function to ensure that every HttpClient gets its own RetryConfig
+ * Default retry configuration for HTTP and HTTP/2 requests. Retries up to 4 times on connection reset and timeout
+ * errors as well as 503 errors. Exposed as a function to ensure that every RequestClient gets its own RetryConfig


Let's backtick these literals.

egilmorez · 2024-06-25T18:05:21Z

src/utils/api-request.ts

+  }
+
+  /**
+   * Sends an HTTP request to a remote server. If the server responds with a successful response (2xx), the returned


There are literals in this long comment that need backticks.

egilmorez · 2024-06-25T18:06:19Z

src/utils/api-request.ts

+   * Sends an HTTP request to a remote server. If the server responds with a successful response (2xx), the returned
+   * promise resolves with an RequestResponse. If the server responds with an error (3xx, 4xx, 5xx), the promise rejects
+   * with an RequestResponseError. In case of all other errors, the promise rejects with a FirebaseAppError. If a
+   * request fails due to a low-level network error, transparently retries the request once before rejecting the


Let's provide a subject noun here -- what entity retries? The server/backend/promise?

egilmorez · 2024-06-25T18:07:24Z

src/utils/api-request.ts

+   * Sends an HTTP/2 request to a remote server. If the server responds with a successful response (2xx), the returned
+   * promise resolves with an RequestResponse. If the server responds with an error (3xx, 4xx, 5xx), the promise rejects
+   * with an RequestResponseError. In case of all other errors, the promise rejects with a FirebaseAppError. If a
+   * request fails due to a low-level network error, transparently retries the request once before rejecting the


Subject here too please, thanks!

egilmorez · 2024-06-25T18:08:18Z

src/utils/api-request.ts


-  constructor(private readonly config: HttpRequestConfig) {
+/**
+ * An adapter class with common functionality needed to extract options and entity data from a RequestConfig.


Looks like a literal.

egilmorez · 2024-06-25T18:08:31Z

src/utils/api-request.ts

    // GET and HEAD requests do not support entity (body) in request.
    return this.method !== 'GET' && this.method !== 'HEAD';
  }
 }

+/**
+ * An adapter class for extracting options and entity data from an HttpRequestConfig.


Another literal.

jonathanedey added 2 commits April 30, 2024 16:52

Refactor api-requets to prepare to handle both HTTP/1.1 and HTTP/2 …

a3e45da

…requests

Added HTTP2 support for sendEach() and sendEachForMulticast` with i…

ad9d780

…ntegration tests

jonathanedey added the release:stage Stage a release candidate label May 9, 2024

jonathanedey added 2 commits May 9, 2024 11:29

fix: lint

991f069

Added unit tests

9100a52

jonathanedey changed the title ~~feat: Add HTTP2 support for sendEach() and sendEachForMulticast~~ feat: Add HTTP2 support for sendEach() and sendEachForMulticast() May 10, 2024

jonathanedey requested a review from lahirumaramba May 10, 2024 19:28

jonathanedey marked this pull request as ready for review May 10, 2024 19:28

Added tests for Http2Client and AuthorizedHttp2Client

1427e7f

github-advanced-security bot found potential problems May 16, 2024

View reviewed changes

jimnor0xF mentioned this pull request May 25, 2024

Add support for FCM http2 API parse-community/parse-server-push-adapter#251

Open

3 tasks

lahirumaramba reviewed May 30, 2024

View reviewed changes

lahirumaramba changed the title ~~feat: Add HTTP2 support for sendEach() and sendEachForMulticast()~~ feat(fcm): Add HTTP2 support for sendEach() and sendEachForMulticast() May 30, 2024

lahirumaramba added the release-note label May 30, 2024

jonathanedey and others added 2 commits June 12, 2024 14:29

Address api review changes and typos.

2003f30

Merge branch 'master' into je-fcm-http2

8b2a881

lahirumaramba requested changes Jun 19, 2024

View reviewed changes

jonathanedey requested a review from egilmorez June 20, 2024 17:27

lahirumaramba assigned egilmorez Jun 25, 2024

egilmorez reviewed Jun 25, 2024

View reviewed changes

jeromegamez mentioned this pull request Jun 26, 2024

Unable to send messaging after updating to 7.10.0 kreait/firebase-php#888

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(fcm): Add HTTP2 support for `sendEach()` and `sendEachForMulticast()` #2550

feat(fcm): Add HTTP2 support for `sendEach()` and `sendEachForMulticast()` #2550

feat(fcm): Add HTTP2 support for sendEach() and sendEachForMulticast() #2550

Are you sure you want to change the base?

feat(fcm): Add HTTP2 support for sendEach() and sendEachForMulticast() #2550

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat(fcm): Add HTTP2 support for `sendEach()` and `sendEachForMulticast()` #2550

feat(fcm): Add HTTP2 support for `sendEach()` and `sendEachForMulticast()` #2550