connection keepAlive does not apply.

Open
#880 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
4/5
Estimated time
3-5 days
Newbie friendliness
30/100
Issue type
Bug
Clarity
Needs clarification
Activity status
Stale
Tech stack
java
Domain
networking

Research direction

Start by comparing FirebaseMessagingClient.sendSingleRequest() behavior in firebase-admin 6.8.0 and 8.1.0, focusing on ApiClientUtils.disconnectQuietly(response) and the Apache HttpClient ConnectionHolder calls described in the report. Reproduce the connection lifecycle with the provided custom keep-alive strategy and verify that connections are retained and recycled according to its configured durations.

Written by the indexing model from the issue text.

Description

needs-triage
[REQUIRED] Step 2: Describe your environment
  • Operating System version: linux
  • Firebase SDK version: firebase-admin-8.1.0.jar
  • Library version: 8.1.0.jar
  • Firebase Product: Cloud Messaging (auth, database, storage, etc)
[REQUIRED] Step 3: Describe the problem

We confirmed that keepAlive is applied when using firebase-admin version 6.8.0.
However, after upgrading to firebase-admin 8.1.0, keepAlive is not applied and the connection is immediately disconnected.

Steps to reproduce:

firebase-admin 6.8.0 FirebaseMessagingClient.sendSingleRequest()
image
(apache httpclient-4.5.11) ConnectionHolder.releaseConnection() is called due to line 129.
At this time, this.released changes from false to true.
After that, ApiClientUtils.disconnectQuietly(response) -> (apache httpclient-4.5.11) ConnectionHolder.abortConnection() on line 132 is called, but the connection is not disconnected because ConnectionHolder.released = true.
image
image

However, in firebase-admin 8.1.0, ConnectionHolder.releaseConnection() is not called, but ConnectionHolder.abortConnection() is called, so the connection is disconnected.
image

What happened? How can we make the problem occur?
Establishing connections is expensive. Therefore, recycling connections for a certain period of time can help improve performance. According to what I have checked, the code of the latest version, 9.2.0, is also the same.

Relevant Code:
private FirebaseMessaging initializeFirebaseMessaging()
        throws IOException {
        CloseableHttpClient httpClient = newHttpClient(proxyHost, proxyPort);

        HttpTransport httpTransport = new ApacheHttpTransport(httpClient);

        FirebaseOptions options = FirebaseOptions.builder()
// need credentials etc
                                                 .setHttpTransport(httpTransport)
                                                 .build();

        FirebaseApp.initializeApp(options);
        return FirebaseMessaging.getInstance();
    }

private CloseableHttpClient newHttpClient(String proxyHost, int proxyPort) {
        HttpClientBuilder httpClientBuilder = ApacheHttpTransport.newDefaultHttpClientBuilder()
                                                                 .evictExpiredConnections()
                                                                 .setKeepAliveStrategy(
                                                                     new CustomConnectionKeepAliveStrategy(
                                                                         TimeUnit.MINUTES.toMillis(3)))
                                                                 .setConnectionTimeToLive(10, TimeUnit.MINUTES)
                                                                 .setDefaultRequestConfig(requestConfig())
                                                                 .disableCookieManagement();
        return httpClientBuilder.build();
    }

public class CustomConnectionKeepAliveStrategy implements ConnectionKeepAliveStrategy {

    public static final CustomConnectionKeepAliveStrategy INSTANCE =
        new CustomConnectionKeepAliveStrategy(TimeUnit.SECONDS.toMillis(50),
            DefaultConnectionKeepAliveStrategy.INSTANCE);

    public static final ConnectionKeepAliveStrategy NO_KEEPALIVE = (response, context) -> 0;

    private static final long KEEP_ALIVE_SAFE_GAP = 500L;
    private final ConnectionKeepAliveStrategy defaultConnectionKeepAliveStrategy;
    private final long defaultKeepAliveTimeoutInMillis;

    public CustomConnectionKeepAliveStrategy(long defaultKeepAliveTimeoutInMillis) {
        this(defaultKeepAliveTimeoutInMillis, DefaultConnectionKeepAliveStrategy.INSTANCE);
    }

    public CustomConnectionKeepAliveStrategy(long defaultKeepAliveTimeoutInMillis,
        ConnectionKeepAliveStrategy defaultConnectionKeepAliveStrategy) {
        Args.notNegative(defaultKeepAliveTimeoutInMillis, "defaultKeepAliveTimeoutInMillis");
        Args.notNull(defaultConnectionKeepAliveStrategy, "defaultConnectionKeepAliveStrategy");
        this.defaultKeepAliveTimeoutInMillis = defaultKeepAliveTimeoutInMillis;
        this.defaultConnectionKeepAliveStrategy = defaultConnectionKeepAliveStrategy;
    }

    public static long adjustKeepAliveTimeout(long keepAliveTimeoutInMillis) {
        return Math.max(0L, (keepAliveTimeoutInMillis - KEEP_ALIVE_SAFE_GAP));
    }

    @Override
    public long getKeepAliveDuration(final HttpResponse response, final HttpContext context) {
        long keepAliveDuration = defaultConnectionKeepAliveStrategy.getKeepAliveDuration(response, context);
        if (keepAliveDuration == -1L) {
            return defaultKeepAliveTimeoutInMillis;
        }
        if (keepAliveDuration > 0L) {
            return adjustKeepAliveTimeout(keepAliveDuration);
        }
        return keepAliveDuration;
    }
Dominant language
Java
Stars
620
Forks
305
Avg merge
3h 23m
Merged PRs (30d)
1

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from firebase/firebase-admin-java

All issues in firebase/firebase-admin-java

Similar issues

More Java issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.