sublimehq / sublime_text

Issue tracker for Sublime Text
https://www.sublimetext.com
807 stars 39 forks source link

Default/exec.py resource leak #3836

Open JoshuaWebb opened 3 years ago

JoshuaWebb commented 3 years ago

Description

There's currently an outstanding bug/issue in Python 3.7+ where non daemon python threads that end naturally but aren't ever checked for liveness e.g. is_alive(), nor join()ed, leak a lock object. See https://bugs.python.org/issue37788

The specifics are an implementation detail of CPython 3.8/threading.py, and the documentation doesn't mention requiring any specific calls to ensure timely resource cleanup of completed threads, but starting a (non daemon) thread places a lock object into a global set (_shutdown_locks) to be "joined" when python shuts down. This lock is removed from the set when you call join(), or if you call is_alive() on a thread after it has completed, but if you don't call either of those, this reference keeps the lock from being garbage collected along with any associated system resources (e.g. a Semaphore handle on Windows) until sublime exits.

It looks like various fixes have already been proposed and at least one of them was merged (and subsequently reverted), so this issue will hopefully end up being resolved automatically in an upcoming version of Python, but it's unclear how soon that will happen.

NOTE: the stderr_thread is being join()ed in read_fileno() (on the stdout_thread), so only the stdout_thread leaks its lock.

Steps to reproduce

  1. Execute a build using the default exec build target.
  2. Leak.

This can be verified in a via the console:

  1. In the console:
    import threading
    threading._shutdown_locks

    {<locked _thread.lock object at 0x000001B131857810>, <unlocked _thread.lock object at 0x000001B131F42CF0>}

  2. Execute build
  3. In the console: threading._shutdown_locks

    {<locked _thread.lock object at 0x000001B131857810>, <unlocked _thread.lock object at 0x000001B131F58930>, <unlocked _thread.lock object at 0x000001B131F42CF0>}

  4. Repeat steps 2/3; observe that the set grows by one lock every time.

This can also be verified externally using tools like Sysinternal's handle.exe; the following example calls handle before doing any builds, and then once in between each build. There are some overhead Semaphores from the first build (33 up to 37), presumably some of the other global variables from threading, but then each subsequent build adds one more Semaphore handle.

05:28:00 /x$ handle -p plugin_host-3.8.exe -nobanner -s
Handle type summary:
  <Unknown type>  : 70
  ALPC Port       : 1
  Desktop         : 1
  Directory       : 2
  Event           : 25
  File            : 7
  IoCompletion    : 3
  IRTimer         : 4
  Key             : 12
  Process         : 1
  Semaphore       : 33
  Thread          : 3
  TpWorkerFactory : 2
  WaitCompletionPacket: 6
  WindowStation   : 2
Total handles: 172
05:28:11 /x$ handle -p plugin_host-3.8.exe -nobanner -s
Handle type summary:
  <Unknown type>  : 71
  ALPC Port       : 1
  Desktop         : 1
  Directory       : 2
  Event           : 25
  File            : 11
  IoCompletion    : 3
  IRTimer         : 4
  Key             : 13
  Process         : 2
  Semaphore       : 37
  Thread          : 3
  TpWorkerFactory : 2
  WaitCompletionPacket: 6
  WindowStation   : 2
Total handles: 183
05:28:17 /x$ handle -p plugin_host-3.8.exe -nobanner -s
Handle type summary:
  <Unknown type>  : 71
  ALPC Port       : 1
  Desktop         : 1
  Directory       : 2
  Event           : 25
  File            : 11
  IoCompletion    : 3
  IRTimer         : 4
  Key             : 13
  Process         : 2
  Semaphore       : 38
  Thread          : 3
  TpWorkerFactory : 2
  WaitCompletionPacket: 6
  WindowStation   : 2
Total handles: 184
05:28:20 /x$ handle -p plugin_host-3.8.exe -nobanner -s
Handle type summary:
  <Unknown type>  : 71
  ALPC Port       : 1
  Desktop         : 1
  Directory       : 2
  Event           : 25
  File            : 11
  IoCompletion    : 3
  IRTimer         : 4
  Key             : 13
  Process         : 2
  Semaphore       : 39
  Thread          : 3
  TpWorkerFactory : 2
  WaitCompletionPacket: 6
  WindowStation   : 2
Total handles: 185
05:28:27 /x$ handle -p plugin_host-3.8.exe -nobanner -s
Handle type summary:
  <Unknown type>  : 71
  ALPC Port       : 1
  Desktop         : 1
  Directory       : 2
  Event           : 25
  File            : 11
  IoCompletion    : 3
  IRTimer         : 4
  Key             : 13
  Process         : 2
  Semaphore       : 40
  Thread          : 3
  TpWorkerFactory : 2
  WaitCompletionPacket: 6
  WindowStation   : 2
Total handles: 186

Expected behavior

The exec target should cleanup after itself, e.g. by marking them as daemon threads, or by joining/checking them after they are finished to remove the lock from the shutdown set.

Actual behavior

A small memory/resource leak for every single build.

Environment

deathaxe commented 3 years ago

Any 3rd-party package can create non-deaomn threads without joining them at any point. Thus fixing the exec module only may not be a sufficient solution to prevent resource leakage.

JoshuaWebb commented 3 years ago

This is true, that's how I discovered the issue in the first place. A more comprehensive fix is to patch the threading module in the version of Python that ships with Sublime, but depending on how long that will take, in the meantime one less leak is one less leak.

deathaxe commented 3 years ago

I am a bit shocked about the issue being known by python community without a fix for about 3 years now.

rwols commented 3 years ago

A more comprehensive fix is to patch the threading module in the version of Python that ships with Sublime

Even better: remove the module! And rely entirely on asyncio. No text editor needs threads for its plugins.