Memory Leak Introduced in 1.7.2 #455

Activity

1. busunkim96

   self-assigned this

   on Mar 9, 2020
2. busunkim96

   added

   priority: p1Important issue which blocks shipping the next release. Will be fixed prior to next release.Important issue which blocks shipping the next release. Will be fixed prior to next release.

   type: bugError or flaw in code with unintended results or allowing sub-optimal usage patterns.Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

   on Mar 9, 2020
3. busunkim96 commented on Mar 10, 2020

   

   busunkim96

   on Mar 10, 2020

   Contributor

   More actions

   @lidizheng Could you provide guidance on the best way to implement gRPC's AuthMetadataPlugin.__call__? I think this library's implementation (using threadpools) was modeled closely on grpc/_auth.py.
4. lidizheng commented on Mar 10, 2020

   

   lidizheng

   on Mar 10, 2020

   Contributor

   More actions

   Okay. This is a bit awkward.

   The code has been out for 3 years, I guess we need to improve its design. I might get back with a PR or a CL? Which one is preferred?
5. busunkim96 commented on Mar 10, 2020

   

   busunkim96

   on Mar 10, 2020

   Contributor

   More actions

   Thanks for the quick response! A PR would be preferred. I'm also happy to tweak this myself if you can point me in the right direction.

   To be completely honest, I have not worked with threading/threadpool, so I was wondering if there was some obvious way to correct this that I was missing. 😅
6. lidizheng commented on Mar 10, 2020

   

   lidizheng

   on Mar 10, 2020

   Last edited by lidizheng

   Contributor

   More actions

   I have a workaround solution that instead of creating a large number of ThreadPoolExecutors, we can have a global one. Like:

   _global_thread_pool = None
   
   class AuthMetadataPlugin(...):
       def __init__(self, ...):
           global _global_thread_pool
           ...
           if _global_thread_pool is None:
               _global_thread_pool = futures.ThreadPoolExecutor()  # Let the system default decide number of max_workers
           self._pool = _global_thread_pool

   It solves the spiking number of threads, but not the memory leak issue entirely.

   ThreadPoolExecutor is slow and buggy. The thread spawned by it is not daemon thread, but thread with special tags that got clean-up by a registered shutdown handler on interpreter exit. It has caused random hang on exit in gRPC Python.

   ---

   EDIT: Use class static variable is better.

   class AuthMetadataPlugin(...):
   
       _AUTH_THREAD_POOL = futures.ThreadPoolExecutor()
   
       def __call__(self, context, callback):
           ...
           future = self._AUTH_THREAD_POOL.submit(self._get_authorization_headers, context)
           ...

   👍React with 👍2Reacted by Richard Belleville and Bu Sun Kim
7. yoshi-automation

   added

   🚨This issue needs some love.This issue needs some love.

   and removed

   🚨This issue needs some love.This issue needs some love.

   on Mar 15, 2020
8. busunkim96

   added a commit that references this issue on Mar 16, 2020

   fix: make threadpool a class var

   ...

   17df134
9. yoshi-automation

   added

   🚨This issue needs some love.This issue needs some love.

   on Mar 16, 2020
10. busunkim96

    closed this as completedon Mar 19, 2020