eclipse-paho / eclipse-paho/paho.mqtt.python
Leaking socketpair filedescriptors to the garbage collector is too easy to happen
- Dominant language
- Python
- Stars
- 2.4k
- Forks
- 742
- Avg merge
- 12d 48m
- Merged PRs (30d)
- 1
Description
The only way to close sockets created by function `_socketpair_compat()` is by an actual deletion of the client object. I think, it would be better (less confusing) to close them also from `loop_stop()`.
The problem with current situation is, that it is really easy to leak a mqtt client object itself to the garbage collector:
```python3
import paho.mqtt.client as mqtt
class Leak:
def __init__(self):
self.mqttc = mqtt.Client()
self.mqttc.on_connect = self.on_connect
def start(self):
self.mqttc.connect("127.0.0.1", 1883)
self.mqttc.loop_start()
def on_connect(self, client, userdata, flags, rc, properties=None):
self.mqttc.subscribe("leaktest", qos=2)
def stop(self):
self.mqttc.disconnect()
self.mqttc.loop_stop()
leak = Leak()
leak.start()
leak.stop()
del(leak)
```
There is object reference loop: directly from object `leak` to `self.mqttc` and then back using the `on_connect()` method. This implies, that both objects are leaked to garbage collector when `del(leak)` is executed. That would be completely fine, if none of the objects is holding any valuable resources. But in case of `paho.mqtt.client`, it also leaks two opened file-descriptors for `_sockpairR` and `_sockpairW`.
Because the default limit of filedescriptors on Linux is only 1024, the garbage collection may happen too late. Here is example which is able to reach this limit using solely mqtt client + some memory allocations to trick the garbage collector.
It allocates a lot of simple objects (approx. 3.7 GB) and then in loop: creates new client, do some work, disconnects and restarts. Beware: this script needs something around 4.1 GB of RAM in our case. I recommend to monitor RAM usage and socket usage (something like `htop` and `watch --interval=0.1 'ss -p | grep python | wc -l'`).
```python3
### WARNING: be sure to have at least 4.5 GB of free RAM before running this
import paho.mqtt.client as mqtt
import time
import socket
import gc
class Leak:
def __init__(self):
self.mqttc = mqtt.Client()
self.mqttc.on_connect = self.on_connect
def start(self):
self.mqttc.connect("127.0.0.1", 1883)
self.mqttc.loop_start()
def on_connect(self, client, userdata, flags, rc, properties=None):
self.mqttc.subscribe("leaktest", qos=2)
def stop(self):
self.mqttc.disconnect()
self.mqttc.loop_stop()
class DummyObjectToTrickGC:
def __init__(self, prev):
self._prev = prev
def do_work(dummies):
for i in range(7000):
dummies.append(DummyObjectToTrickGC(dummies[-1]))
print(gc.get_stats())
dummies = []
dummies.append(DummyObjectToTrickGC(None))
# About 3.7 GB of allocations to trick the garbage collector
# to not run so often for generation 2 objects.
for i in range(3000):
do_work(dummies)
for i in range(512):
leak = Leak()
leak.start()
# Realistic times might be several seconds or even hours, but that has
# no effect on gc's behavior. Using just 0.1 to finish in usable time.
time.sleep(0.1)
# Some memory allocations to move sockets to generation 2 of gc
# while they are still reachable.
do_work(dummies)
leak.stop()
time.sleep(0.1)
```
In our setup (`paho-mqtt==1.5.1` or `paho-mqtt==1.6.1` on Debian Buster with Python 3.7), none of the sockets is closed by gc and whole script crashes (after aprox. 2.5 minutes) with:
```
Traceback (most recent call last):
File "leak.py", line 47, in
File "leak.py", line 16, in start
File "/tmp/socket-leak/venv/lib/python3.7/site-packages/paho/mqtt/client.py", line 1795, in loop_start
File "/tmp/socket-leak/venv/lib/python3.7/site-packages/paho/mqtt/client.py", line 291, in _socketpair_compat
File "/usr/lib/python3.7/socket.py", line 212, in accept
OSError: [Errno 24] Too many open files
```
I think this script may resemble not-so-unreasonable application which wants to restart almost everything (including the mqtt client) on some error, but still want avoid restarting whole python interpreter and keep few gigabytes of data in memory.
I am not sure, if adding `self._reset_sockets(sockpair_only=True)` at the end of `loop_stop()` is sufficient fix for every supported usage of the library. (It should not be enough at least for the obsolete `loop()`.) I also do not know if the similar issue is not present with the main socket when the error is caused by the remote side.
Thank you for any thoughts on this.
Contributor guide
Assessment
This issue has not been assessed yet.