Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retire netifaces #1123

Closed
qstokkink opened this issue Apr 4, 2023 · 6 comments · Fixed by #1133
Closed

Retire netifaces #1123

qstokkink opened this issue Apr 4, 2023 · 6 comments · Fixed by #1133
Assignees
Labels
priority: low Should be addressed at some point in the future

Comments

@qstokkink
Copy link
Collaborator

qstokkink commented Apr 4, 2023

Currently, we use netifaces to retrieve the network interfaces of a machine. We use this to determine the LAN address in the Endpoint class.

There are three reasons to remove netifaces from our code base:

  1. netifaces does not install easily for Python 3.11 on Windows systems.
  2. The code that uses netifaces is old and messy (also, its removal has been planned since 2016, in the second-oldest PR that is still open on Dispersy).
  3. IPv8 can work without netifaces. A workaround already exists in our code.
  4. The netifaces repository has been without a maintainer since 2021.

There is one reason not to remove netifaces:

  1. The workaround to avoid netifaces does not work as well.

We should look into making our workaround more robust so we can retire netifaces. As this code is a decade old (back when we were using Python 2.4 and Python 2.5), we should also investigate using new features in the standard Python library that provide the same functionality.

Historical context: in the past (16 years ago!), Tribler would recognize its WAN addresses by running commands with os.popen 😃 . This is what netifaces originally replaced. Trivia: we also had upnp back then.

@qstokkink qstokkink added the priority: low Should be addressed at some point in the future label Apr 4, 2023
@qstokkink
Copy link
Collaborator Author

I asked for volunteers (thanks everyone who submitted results!) to test a pure-socket workaround for netifaces. The results are as follows:

  • For five out of six tested machines the IPv4 addresses found by the pure-socket implementation match netifaces perfectly.
  • For one machine the pure-socket implementation completely fails to find any non-loopback addresses, while netifaces succeeds.
  • The IPv6 addresses hardly match.

So far, I believe netifaces still adds too much value to be dropped. Even the workaround for just IPv4 addresses is imperfect.

[Click me!] Workaround and reference implementation to find IPv4 addresses.

socket implementation:

set([i[4][0] for i in socket.getaddrinfo(socket.gethostname(), None) + socket.getaddrinfo(None, 0) if i[4][0].find(".") != -1])

netifaces implementation:

set(sum([[v[0]['addr'] for v in netifaces.ifaddresses(itf).values() if v[0]['addr'].find(".") != -1] for itf in netifaces.interfaces()], []))

@qstokkink
Copy link
Collaborator Author

From the netifaces.c code I gathered that the majority of the work netifaces does (in Windows) is handled by a single library call to GetAdaptersAddresses. I figured that using ctypes to bind this single call would be a nice short little pastime for my day off. I was wrong. I did get the thing to run (gist/b3332a5832f07e81c651a84612a196f8) but this was very painful.

@qstokkink
Copy link
Collaborator Author

The other two ways that netifaces retrieves the addresses (for UNIX) seem to be getifaddrs() and ioctl(). An answer on StackOverflow gives me hope that the former is relatively easy to bind and the latter is in the Python standard library (no ctypes trickery needed). We should be able to make "netifaces at home" with pure Python, no compilation needed.

@kozlovsky
Copy link
Contributor

In #1127, I described the performance issue caused by calls to netifaces.ifaddresses and netifaces.interfaces. While it can influence your decision to retire netifaces, I would like to clarify that for #1127, the netifaces library does not look inherently problematic. The issue arises from the frequency of its function invocations, occurring several hundred times per second. It's plausible that substituting netifaces with another library might not result in a significant speed enhancement, as the call speed would likely be comparable.

@kozlovsky
Copy link
Contributor

kozlovsky commented May 19, 2023

@qstokkink What is your opinion about the ifaddr pure-python library? Can it replace netifaces in ipv8?

@qstokkink
Copy link
Collaborator Author

@kozlovsky I don't hate it but I don't love it either. It's pretty close to what I'm working on myself. However, my concerns are three-fold:

  1. Memory management for the Windows GetAdaptersAddresses() call doesn't follow the documentation.
  2. We have to make/maintain boilerplate code for yet another dependency that doesn't quite provide what we need.
  3. The requirements file lists the following and I don't think we need any of it:
black;implementation_name=="cpython"
mypy;implementation_name=="cpython"
# netifaces only provides 64-bit Windows wheels for Python 3.6 and 3.7 and we use 64-bit CI builds
netifaces;python_version=='3.7' and platform_system=='Windows' or platform_system!='Windows'
pytest
pytest-cov

For the record, I still don't hate netifaces either but it does not compile for Windows 11 out-of-the-box (using pip) and it has been unmaintained for 2 years.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: low Should be addressed at some point in the future
Development

Successfully merging a pull request may close this issue.

2 participants