firebase-admin-python icon indicating copy to clipboard operation
firebase-admin-python copied to clipboard

urllib3 connection pool full using messaging.send_each_for_multicast()

Open filippotp opened this issue 2 years ago • 26 comments

  • Operating System version: Ubuntu 22.04 (Heroku)
  • Firebase SDK version: 6.2.0
  • Firebase Product: FCM
  • Python version: 3.11.4
  • Pip version: 23.2.1

I'm using FCM to send multicast messages from my Django app running on Heroku.

Since messaging.send_muticast() function is deprecated, I changed my code by using messaging.send_each_for_muticast(), as stated in the deprecation warning.

After pushing the new code to production, I often see multiple warnings in the Heroku application logs when the app sends messages: WARNING:urllib3.connectionpool:Connection pool is full, discarding connection: fcm.googleapis.com. Connection pool size: 10

Editing my code back to using messaging.send_muticast() seems to solve the issue.

filippotp avatar Aug 09 '23 16:08 filippotp

I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.

google-oss-bot avatar Aug 09 '23 16:08 google-oss-bot

Hi @filippotp,

Thanks for reporting the issue!

According to the answers in https://stackoverflow.com/questions/53765366/urllib3-connectionpool-connection-pool-is-full-discarding-connection, seeing WARNING:urllib3.connectionpool:Connection pool is full, discarding connection: fcm.googleapis.com. Connection pool size: 10 does not mean that there is any problem with messaging.send_each_for_multicast(). As long as messaging.send_each_for_multicast() returns a BatchResponse without any failures in it, then the multicast_message should be sent to all the tokens successfully.

Here's the reason why those warnings only show up in the logs when messaging.send_each_for_multicast() is used:

Unlike messaging.send_multicast, the underlying implementation of messaging.send_each_for_multicast() uses concurrent.futures.ThreadPoolExecutor to start multiple threads to send to tokens via urllib3 in parallel. We set the maximum number of threads that ThreadPoolExecutor can start, that is max_workers, to the number of the tokens in the multicast_message. That means if a multicast_message contains 50 tokens, then ThreadPoolExecutor may start at most 50 threads. However, the maxsize for urllib3.connectionpool is not configured so it is the default value, 10. Then https://stackoverflow.com/a/66671026 helps explain everything:

For example, if your maxsize is 10 (the default when using urllib3 via requests), and you launch 50 requests in parallel, those 50 connections will be performed at once, and after completion only 10 will remain in the pool while 40 will be discarded (and issue that warning).

So if you only send multicast_message with no more than 10 tokens, I think the warning logs should be gone. However, the warning logs don't really matter. messaging.send_each_for_multicast() should still work properly regardless of the warnings. We may optimize the implementation of messaging.send_each_for_multicast() to start less threads in the future and may be able to get rid of those warnings, but before that happens, please continue migrating to messaging.send_each_for_multicast().

Doris-Ge avatar Aug 18 '23 09:08 Doris-Ge

Any updates on letting messaging.send_each and messaging.send_each_for_multicast start less threads? Its not obvious to me why starting a thread per token in the multicast message is a good idea. The number of tokens that one wants to send the message to most likely has nothing to do with the desire for concurrency (which depends on system resources, etc.)

Note that calling messaging.send_each_for_multicast multiple times (which is what I do now) is a sub-optimal solution.

Maybe the SDK can be extended to allow us to configure the max_workers to use when calling messaging.send_each and messaging.send_each_for_multicast?

mortenthansen avatar Jun 07 '24 08:06 mortenthansen

+1 on @mortenthansen comment, we got bit by this, sending push by batches of 500 messages, only to realise len(input) was used as the pool_size for the ThreadPool. I really can't figure out a good rationale for this. Spawning 500 threads (which I don't think will happen since python ThreadPool reuses free ones before spawning new ones), isn't cost free and at the very least should be documented as such.

Klemenceo avatar Jun 11 '24 08:06 Klemenceo

Hi @filippotp,

Thanks for reporting the issue!

According to the answers in https://stackoverflow.com/questions/53765366/urllib3-connectionpool-connection-pool-is-full-discarding-connection, seeing WARNING:urllib3.connectionpool:Connection pool is full, discarding connection: fcm.googleapis.com. Connection pool size: 10 does not mean that there is any problem with messaging.send_each_for_multicast(). As long as messaging.send_each_for_multicast() returns a BatchResponse without any failures in it, then the multicast_message should be sent to all the tokens successfully.

Here's the reason why those warnings only show up in the logs when messaging.send_each_for_multicast() is used:

Unlike messaging.send_multicast, the underlying implementation of messaging.send_each_for_multicast() uses concurrent.futures.ThreadPoolExecutor to start multiple threads to send to tokens via urllib3 in parallel. We set the maximum number of threads that ThreadPoolExecutor can start, that is max_workers, to the number of the tokens in the multicast_message. That means if a multicast_message contains 50 tokens, then ThreadPoolExecutor may start at most 50 threads. However, the maxsize for urllib3.connectionpool is not configured so it is the default value, 10. Then https://stackoverflow.com/a/66671026 helps explain everything:

For example, if your maxsize is 10 (the default when using urllib3 via requests), and you launch 50 requests in parallel, those 50 connections will be performed at once, and after completion only 10 will remain in the pool while 40 will be discarded (and issue that warning).

So if you only send multicast_message with no more than 10 tokens, I think the warning logs should be gone. However, the warning logs don't really matter. messaging.send_each_for_multicast() should still work properly regardless of the warnings. We may optimize the implementation of messaging.send_each_for_multicast() to start less threads in the future and may be able to get rid of those warnings, but before that happens, please continue migrating to messaging.send_each_for_multicast().

@filippotp does this means that send_each_for_multicast with 500 messages will send all push notifications, regardless the warnings (will not discard 490 messages out of 500 and send only 10 per batch)?

Thank you for your answer in advance :)

milemik avatar Jul 26 '24 09:07 milemik

@milemik I think you are referring to @Klemenceo's comment.

Anyway, in my personal case the issue has never occurred again after updating firebase-admin to 6.3.0 and definitively migrating to messaging.send_each_for_multicast().

filippotp avatar Jul 26 '24 15:07 filippotp

@milemik I think you are referring to @Klemenceo's comment.

Anyway, in my personal case the issue has never occurred again after updating firebase-admin to 6.3.0 and definitively migrating to messaging.send_each_for_multicast().

Yes, sorry questtion was for @Doris-Ge :) I have version 6.5.0 and I can see these warnings, in my logs of django (Celery) project. Just want to understand if all of my push notifications will be sent regardless of this warning.

Thank you 😄

milemik avatar Jul 26 '24 18:07 milemik

I have version 6.5.0 and I can see these warnings, in my logs of django (Celery) project, also because of spawning 500 threads the workers consumes 100% CPU machine which affects other workers.

What can be the solution for this? @milemik @filippotp

ankit-wadhwani-hb avatar Aug 22 '24 08:08 ankit-wadhwani-hb