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

Avoid race conditions when starting Socks5 servers and REST API by allowing OS to choose a free port #7565

Merged
merged 4 commits into from
Aug 10, 2023

Conversation

kozlovsky
Copy link
Contributor

@kozlovsky kozlovsky commented Aug 7, 2023

This PR fixes #7564. It stops using default_network_utils.get_random_free_port() and passes port value zero when starting a server to allow OS to choose a free port without race conditions.

The PR consists of several commits:

  1. The first commit stops using get_random_free_port() when starting a new Socks5Server
  2. The second commit renames events_manager.connect(...) to events_manager.connect_to_core(...). The reason for this is the connect name is overused: the same name is used for subscribing to PyQt signals, for opening UdpTrackerSession, and for connecting to the sqlite3 database in ProcessManager, so it is better to have a distinct name to find all places when the specific method is used easily.
  3. The third commit changes the logic of how REST API Manager gets the port. After the PR changes, Tribler GUI no longer tries to call get_random_free_port() and detect the free port for Tribler Core's REST API usage. Instead, Tribler Core requests a free port directly from OS when starting the server and then passes the port value to Tribler GUI. Now it is unnecessary to have a loop over a range of ports on the Coree side and attempt to start a server with a possible retry, as the suitable port is available from the first attempt.
  4. The fifth commit removes the REST API port from Tribler GUI settings. For most users, settings the specific REST API port value is unnecessary, as it is an internal details of Tribler's GUI/Core communication. If it is necessary for sone user to set a specific REST API port, he can do it by setting CORE_API_PORT environment variable (as specified in the documentation). In my opinion, UI settings should only display settings that can actually be important to many Tribler users, and it seems that REST API port value does not belong to this category.

As a result:

  1. The logic of starting the servers was simplified and streamlined
  2. The possible reasons for race conditions are eliminated
  3. The retry logic when starting the REST API was removed as unnecessary
  4. Tribler settings in GUI were simplified, and the unnecessary option was removed

Previously, the documentation said 20100 is a default port value for Swagger UI and REST API. It is not entirely true, as the port number was taken from a range. This part of the documentation is clarified: the user should specify the port value in the CORE_API_PORT environment variable to start the REST API and Swagger UI on a specific port.

settings["listen_interfaces"] = "0.0.0.0:%d" % libtorrent_port

# We do not specify the port in settings dict as we allow libtorrent to automatically choose the port
# settings["listen_interfaces"] = "0.0.0.0:%d" % libtorrent_port
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allowing users to specify a port for torrent traffic is critical functionality for some users. The other ports probably don't matter as much but I'd leave the libtorrent port config option as-is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this link!

Thinking about this, the current Tribler logic regarding the libtorrent port number handling does not look correct to me. This is how, in my understanding, the current logic of DownloadManager.create_session() works:

  1. Initially, in LibtorrentSettings, the port value is None by default (and a user can set a specific value):
class LibtorrentSettings(TriblerConfigSection):
    ...
    port: Optional[int] = None
    ...
  1. The value is read from config, and an empty value is replaced with a random free port:
libtorrent_port = self.config.port or default_network_utils.get_random_free_port()
  1. The resulting port value is passed to libtorrent:
settings["listen_interfaces"] = "0.0.0.0:%d" % libtorrent_port
...
self.set_session_settings(ltsession, settings)
  1. The libtorrent is requested to listen on a port in a range, starting from a specified port value:
ltsession.listen_on(libtorrent_port, libtorrent_port + 10)
  1. Then, if the actual port used by libtorrent is different from the port that was initially specified, the actual port value is stored in the config:
if libtorrent_port != ltsession.listen_port() and store_listen_port:
    self.config.port = ltsession.listen_port()

(the store_listen_port parameter value is True by default and is not specified when the DownloadManager.create_session method is called).

From this, my conclusion is the following: even when a user specifies the libtorrent port value in settings, it is possible that the value will be changed to a different value (at step 3) if the original port is already occupied. Then the value provided by a user will be overridden in settings by a new value without any confirmation by a user.

From what I currently understand, the correct logic should work in the following way:

  1. If the port value is explicitly specified by a user, it should be used. If the specified port is already occupied, Tribler should not use a different port silently. Instead, it should notify the user that the specified port is unavailable and stop.
  2. If the value is not specified, Tribler should allow libtorrent to choose the port automatically. In that case, Tribler should not store the randomly assigned port value in LibtorrentSettings, as it is not guaranteed that the same port will be available next time, and it is not important to use the same port next time. Only the port value explicitly specified by a user should go to LibtorrentSettings.

@qstokkink what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good 👍 That's what I would expect from my torrent client.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the offline discussion with @xoriole we decided to extract libtorrent port handling changes to a separate PR

@kozlovsky kozlovsky force-pushed the dynamic-ports branch 2 times, most recently from 79a0efc to fcf7877 Compare August 8, 2023 10:04
@kozlovsky kozlovsky marked this pull request as ready for review August 8, 2023 13:23
@kozlovsky kozlovsky requested review from a team and xoriole and removed request for a team August 8, 2023 13:23
doc/restapi/introduction.rst Outdated Show resolved Hide resolved
src/tribler/core/start_core.py Show resolved Hide resolved
src/tribler/gui/network/request_manager.py Outdated Show resolved Hide resolved
src/tribler/gui/network/tests/test_request_manager.py Outdated Show resolved Hide resolved
@kozlovsky kozlovsky changed the title Avoid race conditions when starting any servers (Socks5, libtorrent, REST API) by allowing OS to choose a free port Avoid race conditions when starting Socks5 servers and REST API by allowing OS to choose a free port Aug 10, 2023
@kozlovsky kozlovsky merged commit 0e0fcaf into Tribler:main Aug 10, 2023
15 of 16 checks passed
@kozlovsky kozlovsky deleted the dynamic-ports branch August 10, 2023 10:26
kozlovsky added a commit that referenced this pull request Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[7.13.0-RC3] OSError in SocksServersComponent: [Errno 10048] error while attempting to bind on address
3 participants