ethtool fd leaks #1081
ethtool fd leaks #1081
Conversation
svinota
force-pushed
the
svinota-fd-leaks-1079
branch
from
a2f0f36 to
a8bda3e
Compare
| def __init__(self): | ||
| self._with_ioctl = IoctlEthtool() | ||
| self._with_nl = NlEthtool() | ||
| self._with_nl.module_err_level = 'debug' | ||
| self._is_nl_working = self._with_nl.is_nlethtool_in_kernel() | ||
|
|
||
| def __enter__(self): |
There was a problem hiding this comment.
Isn't it better to define this in one of the base classes ?
There was a problem hiding this comment.
Netlink classes have it defined, but Ethtool is a bit separate thing using composition (netlink | ioctl). We can later create one common context manager utility class.
tests/test_linux/test_ethtool.py
Outdated
|
|
||
|
|
||
| def get_fds(): | ||
| return set(os.listdir(f'/proc/{os.getpid()}/fd')) |
There was a problem hiding this comment.
may fail because listing a directory requires file descriptor. Unstable test. How to do better ? open() directory and then list the directory by passing file descriptor instead of path. Then omit the descriptor of the directory from the resulting set.
tests/test_linux/test_ethtool.py
Outdated
| def get_fds(): | ||
| fd = os.open(f'/proc/{os.getpid()}/fd', os.O_RDONLY) | ||
| try: | ||
| return set(os.listdir(fd)) |
Explicitly open/use/close the proc/../fd directory fd
svinota
force-pushed
the
svinota-fd-leaks-1079
branch
from
886d983 to
b7b4452
Compare
|
lgtm |
| def test_pipe_leak(): | ||
| fds = get_fds() | ||
| etht = Ethtool() | ||
| etht.close() |
There was a problem hiding this comment.
HUH. not calling .close() will leak descriptors anyway ? You have to add __del__ to classes that use os.open() / os.pipe() in class constructors.
There was a problem hiding this comment.
Personally I prefer resources to be freed explicitly, putting anyting in __del__ requires a lot of checks and exception handling to guarantee no errors during the GC work.
But I'm to try to look what we can do there.
There was a problem hiding this comment.
@svinota just close the resources opened explicitly.
from contextlib import suppress
class Xxx:
def __init__(self):
self.__x, self.__y = os.pipe()
def close():
# i.e. check every resource if it is closed already, close and clear the variable somehow.
if self.__x !=1:
os.close(self.__x)
self.__x = -1 # not 'None' because of typing.
def __del__(self):
super().__del__() # is it required?
self.close()There was a problem hiding this comment.
@svinota Another way:
import os
class FdWrapper(int):
def __del__(self):
self.close()
def close(self):
if not hasattr(self, '__closed'):
try:
print('here')
os.close(self)
except OSError:
pass # TODO: log ?
finally:
setattr(self, '__closed', True)
def MyPipe() -> tuple[FdWrapper, FdWrapper]:
x, y = os.pipe()
return FdWrapper(x), FdWrapper(y)
x, y = MyPipe()
os.write(y, b'qwe')
x.close()P.S. updated. I don't use a constructor because it's hard to do it when subclassing int. But you may prefer not to subclass, but create parentless class and add .fileno()
Bug-Url: #1079