feat: Add HTTP/2 enabled transport as default transport #979

jonathanedey · 2024-08-20T14:27:54Z

This changes the default HttpTransport to an HTTP/2 enabled Apache 5 HttpClient.

…on tests.

jonathanedey · 2024-08-20T14:44:30Z

src/test/java/com/google/firebase/internal/ApacheHttp2TransportIT.java

+  // Sets a 1 second delay before response
+  private static final String DELAY_URL = "https://nghttp2.org/httpbin/delay/1";
+  private static final String POST_URL = "https://nghttp2.org/httpbin/post";


To test the timeouts I used this live server to simulate delay which is not ideal. What are your thoughts on this?

emindeniz99 · 2024-08-20T15:41:21Z

Thank you for your efforts. I think, if a new transport is implemented and used instead of tested built-in google http client, they should be very well tested. we can copy test cases from this not merged PR to this PR in order to test new http2transport layer.

https://github.com/googleapis/google-http-java-client/pull/1960/files#diff-f6e7e17e639b08337930ac900026abfe1531b7c7e9bea3b6866835526ed1fa81

googleapis/google-http-java-client#1960

emindeniz99 · 2024-08-20T15:47:22Z

It fixes one of the big problem with current state of sdk but it is could be efficient if thread per request structure of library is changed. CallOp causes blocking a thread per request.

This is better, it does not open many sockets, etc. HTTP2 is used. It solves our main problem.
But the fcm library creates a thread for each request, for example sending 6 batch requests with each batch consisting of 500 device tokens. 6*500= 3k threads will be created.
As I know, each thread requires 1 MB memory, this is another problem.
Copied from: #834 (comment)

… messages.

jonathanedey · 2024-08-23T18:29:36Z

Hi @emindeniz99, Thanks for your patience on this!

Went ahead and mirrored most of those tests here.

The issue of threads is something we have been looking into along side http2. The main culprit being the default ThreadExecutor (although it reuses idle threads) has no thread limit which causes more harm than good. We're planning to address this in a separate PR.

Ideally we would want to make full use of Apache HttpClient 5's async clients and use CompletableFutures instead of a thread per request. However from my understanding this requires the google http client to also support async clients or an effort on our side to modify their classes to do so.

This transport in itself is a bandaid solution until async clients required for HTTP/2 are supported in google http client.

honorhs · 2024-08-27T04:52:44Z

Thank you for your efforts in providing the HTTP/2 functionality.
It would be beneficial to have a report comparing the performance of the version with HTTP/2 applied and the previous version. This could include metrics such as system resources and processing completion time, under the assumption of sending a large number of push messages

lahirumaramba

LGTM! I added a few comments. We can also discuss offline
Thanks!

lahirumaramba · 2024-09-03T16:20:54Z

src/main/java/com/google/firebase/internal/ApacheHttp2AsyncEntityProducer.java

+import org.apache.hc.core5.http.nio.AsyncEntityProducer;
+import org.apache.hc.core5.http.nio.DataStreamChannel;
+
+@SuppressWarnings("deprecation")


Which deprecation is this? Maybe we should add a note

This is related to the google api client's StreamingContent. Removed the suppression to match other files where we don't suppress this warning.

lahirumaramba · 2024-09-03T16:21:53Z

src/main/java/com/google/firebase/internal/ApacheHttp2AsyncEntityProducer.java

+  ByteBuffer getBytebuf() {
+    return bytebuf;
+  }
+}


nit: new line

lahirumaramba · 2024-09-03T16:22:04Z

src/main/java/com/google/firebase/internal/ApacheHttp2AsyncEntityProducer.java

+          content.writeTo(baos);
+        } catch (IOException e) {
+          writeFuture.completeExceptionally(e);
+          // failed(e);


nit: can we remove this now?

This should have been uncommented. Fixed that and fixed an issue where exceptions were not being re-thrown.

lahirumaramba · 2024-09-03T16:24:04Z

src/main/java/com/google/firebase/internal/ApacheHttp2Transport.java

+        new PoolingAsyncClientConnectionManager();
+    connectionManager.setMaxTotal(100);
+    connectionManager.setDefaultMaxPerRoute(100);
+    connectionManager.closeIdle(TimeValue.of(30, TimeUnit.SECONDS));


Should we add a note on how we decided on these limits? 100 max connections and 30 seconds timeout?

Good point, I changed the settings here to match the google api client's apache http transport and added a comment stating that.

Also correctly limited the max concurrent streams to 100 and added a comment on the reasoning for this.

Also added more additional comments throughout this file.

lahirumaramba · 2024-09-03T16:26:24Z

src/test/java/com/google/firebase/internal/ApacheHttp2TransportIT.java

+      ImmutableMap.<String, Object>of("foo", "bar");
+
+  // Sets a 5 second delay before response
+  private static final String DELAY_URL = "https://nghttp2.org/httpbin/delay/5";


can we add a note explaining why?

lahirumaramba · 2024-09-03T16:31:59Z

src/main/java/com/google/firebase/internal/ApiClientUtils.java

@@ -88,6 +88,6 @@ public static JsonFactory getDefaultJsonFactory() {
  }

  public static HttpTransport getDefaultTransport() {
-    return Utils.getDefaultTransport();
+    return new ApacheHttp2Transport();


Are we dropping the google api client DefaultTransport and moving to the new apache client? This change affects the core SDK right?

This doesn't require core SDK changes since we are still using a base HttpTransport and developers can still pass in their custom transports instead. However since the underlying transport is changing, there are still minor side effects in how requests are sent under the hood.

Keys ones being:

Timeouts

Low level request exceptions

For non http/2 backend service endpoints, requests will fallback to use http/1. As we discussed, we can decide on whether this should replace the current default transport or simply expose the new http/2 transport for developers to add manually, after we verify proxy functionality.

i build this yesterday and we are testing it in production .. :) since we completely overlooked the change.
had conflicts with pushy dependencies, so this was the last option.

im basically using your apachehttp2transport with more connections. any tip on max output settings? the 9.3.0 with sendEachForMulticastAsync required to use semaphores or it crashed our push server.

we are a soccer company and need to send out like ~80k pushes regulary as fasts as possible.
this solution seem to be stable in terms of pushes send & received, but processing seems kinda slow. and its crazy hard to get settings and processing queues correct.

i build this yesterday and we are testing it in production .. :) since we completely overlooked the change. had conflicts with pushy dependencies, so this was the last option.

im basically using your apachehttp2transport with more connections. any tip on max output settings? the 9.3.0 with sendEachForMulticastAsync required to use semaphores or it crashed our push server.

we are a soccer company and need to send out like ~80k pushes regulary as fasts as possible. this solution seem to be stable in terms of pushes send & received, but processing seems kinda slow. and its crazy hard to get settings and processing queues correct.

you can use current library for creating auth token and body, after that, send via spring webclient or reactor netty.
#834 (comment)
you check the code i shared on the comment and read the other issue for details.

hi emindeniz99 thanks for the reply. i tried it before but run in dependencies nightmare with pushy.

our issue is the delay. it used to be 1-2s till everything was out, now its like 1s - 30s on large loads.
firebase seems to have added some anti burst behavior. with warmups etc. which is fine for normal stuff, but people want to know when a goal was shot asap.

like can we get out 80k~ in a second?

@SmikeSix2 Have you checked the GCP quota? Also, you're correct—the FCM endpoints are unreliable, and their response times fluctuate. We're using Pushy as well. FCM, WebClient, Reactor Netty, and Pushy work well together in our setup. We can reach 150k rpm on single pod.(I do not remember specs).

@SmikeSix2 80k rps is very high, 5million rpm :) fcm does not like it. Did not you reach the quota? Do you have 5m+ limit for fcm? Did you check on gcp console?

hehe its up to 350k rps... in bad scenarios. for example when the national team plays. old solution did fine with it, I was always complimenting firebase. but yeah probably need to switch to topics for the large teams.

my colleague has the access ill ask around, thanks for your input!

diegomarquezp · 2024-09-05T23:38:29Z

This transport in itself is a bandaid solution until async clients required for HTTP/2 are supported in google http client.

cc: @burkedavison @ldetmer

jonathanedey added 6 commits August 14, 2024 15:52

Added HTTP/2 enabled transport and made it default.

e0f3e21

fix: Use internal default transport

93016eb

Added test coverage for timeouts

1541ad0

fix: lint

d78632c

fix: Timeout tests no longer remove Firebase Apps for other integrati…

81246d9

…on tests.

Merge branch 'master' into je-http2-default

a86c325

jonathanedey added release-note release:stage Stage a release candidate labels Aug 20, 2024

jonathanedey requested a review from lahirumaramba August 20, 2024 14:36

jonathanedey commented Aug 20, 2024

View reviewed changes

lahirumaramba self-assigned this Aug 20, 2024

Mirror tests from google java client and added more descriptive error…

ff1e4fb

… messages.

jonathanedey added 4 commits August 27, 2024 10:56

fix: Remove NO_CONNECT_URL

36927d9

debug IT Error

ed88849

debug

21bfa8a

debug

f9e8576

lahirumaramba approved these changes Sep 3, 2024

View reviewed changes

lahirumaramba assigned jonathanedey and unassigned lahirumaramba Sep 3, 2024

jonathanedey added 3 commits September 3, 2024 12:41

debug

3368eb2

debug

5797b48

debug

0c37ba4

This was referenced Sep 4, 2024

a large number of threads will be created when use sendEach api #994

Closed

[FR] Add HTTP2 support for sendEach() and sendEachForMulticast() #973

Open

Is sendEachForMulticast thread safe #950

Closed

diegomarquezp mentioned this pull request Sep 11, 2024

chore(apache-v5): confirm behavior of connect timeout googleapis/google-http-java-client#2014

Draft

jonathanedey and others added 9 commits September 12, 2024 15:31

Remove test debug

1881f28

Address review comments

74c57b7

Use local server to test connect timeout and fix lint

ceac56c

Fix testConnectTimeoutGet test

18365a7

Fix lint

92d3112

remove deplicate tests

73e1355

fix: catch java.net.SocketTimeoutException

165ff59

Proxy tests

11aab96

Merge branch 'master' into je-http2-default

be9e869

jonathanedey requested a review from lahirumaramba September 23, 2024 20:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add HTTP/2 enabled transport as default transport #979

feat: Add HTTP/2 enabled transport as default transport #979

jonathanedey commented Aug 20, 2024

jonathanedey Aug 20, 2024

emindeniz99 commented Aug 20, 2024 •

edited

Loading

emindeniz99 commented Aug 20, 2024

jonathanedey commented Aug 23, 2024

honorhs commented Aug 27, 2024

lahirumaramba left a comment

lahirumaramba Sep 3, 2024

jonathanedey Sep 17, 2024

lahirumaramba Sep 3, 2024

jonathanedey Sep 17, 2024

lahirumaramba Sep 3, 2024

jonathanedey Sep 17, 2024

lahirumaramba Sep 3, 2024

jonathanedey Sep 17, 2024

lahirumaramba Sep 3, 2024

jonathanedey Sep 17, 2024

lahirumaramba Sep 3, 2024

jonathanedey Sep 17, 2024

SmikeSix2 Sep 19, 2024

emindeniz99 Sep 19, 2024

SmikeSix2 Sep 20, 2024

emindeniz99 Sep 20, 2024

emindeniz99 Sep 20, 2024

SmikeSix2 Sep 20, 2024

diegomarquezp commented Sep 5, 2024

feat: Add HTTP/2 enabled transport as default transport #979

Are you sure you want to change the base?

feat: Add HTTP/2 enabled transport as default transport #979

Conversation

jonathanedey commented Aug 20, 2024

Choose a reason for hiding this comment

emindeniz99 commented Aug 20, 2024 • edited Loading

emindeniz99 commented Aug 20, 2024

jonathanedey commented Aug 23, 2024

honorhs commented Aug 27, 2024

lahirumaramba left a 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

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

diegomarquezp commented Sep 5, 2024

emindeniz99 commented Aug 20, 2024 •

edited

Loading