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

Fix UDP can't connect warnings and add IP cache on the way #5329

Merged
merged 91 commits into from
May 5, 2023

Conversation

tearfur
Copy link
Member

@tearfur tearfur commented Mar 30, 2023

Fixed #4517, possibly fixes #86 too.

As far as I can tell, the reason this is happening is because tr_net_hasIPv6() will only disable UDP6 for Transmission (by not creating a UDP6 socket) when the IPv6 module is not loaded into the kernel (for Linux at least). It does not stop Transmission from trying to connect to IPv6 peers via UDP when the IPv6 module is loaded, but no global IPv6 connectivity is available. However, it needs to stay this way in case a peer is found by LPD.

if (!tr_net_hasIPv6(udp_port_))
{
// no IPv6; do nothing
}

The same can happen for IPv4 if the user only has IPv6 (I think).

So I figured the best way to fix this is to check if the system has a source address that can connect to the public internet in tr_session::tr_udp_core::sendto(), which is called for every buffer that needs to be sent over UDP.

Thus, this PR ballooned into adding a whole new infrastructural class, which is an IP cache that stores 3 pieces of info for each IP protocol:

  1. Whether this system supports this IP protocol. tr_session.has_ip_protocol()
  2. The source address used by Transmission that provides connectivity to the public internet. tr_session.globalSourceIP()
  3. The global address that Transmission is being seen as by other peers. tr_session.globalIP()

If I did everything right, then:

  • tr_net_hasIPv6() can be replaced by tr_session.has_ip_protocol() (This will be for a future PR)
  • Apps using libtransmission can make use of the cache to tell users the global IP that they are being seen as, which is a function I wish I had at some point. (Not implemented)
  • And of course, tr_udp_core knows not to bother sending stuff to global addresses when we have no public internet connectivity (i.e. no source address cached). (Already in this PR)

@nevack
Copy link
Member

nevack commented Mar 30, 2023

I also don't know how to hide the changes already merged into main. 😨

Github correctly show only newly modified files.
I think you can git fetch origin main:main and then git rebase origin/main.
Of course then you have to git push --force origin <branch-name>.

@tearfur
Copy link
Member Author

tearfur commented Mar 30, 2023

I also don't know how to hide the changes already merged into main. 😨

Github correctly show only newly modified files. I think you can git fetch origin main:main and then git rebase origin/main. Of course then you have to git push --force origin <branch-name>.

I think it did not work in local because my local main branch was outdated. Anyways I fixed it now. Thanks!

@nevack
Copy link
Member

nevack commented Mar 30, 2023

I also don't know how to hide the changes already merged into main. 😨

Github correctly show only newly modified files. I think you can git fetch origin main:main and then git rebase origin/main. Of course then you have to git push --force origin <branch-name>.

I think it did not work in local because my local main branch is outdated. Anyway I fixed it now. Thanks!

I guessed this too, that's why I suggested fetch first 😉

@tearfur
Copy link
Member Author

tearfur commented Apr 1, 2023

@ckerr I think I am dealing with a race condition during tr_session's initialisation after adding a new field to it. Would aprreciate some help identifying it.

By some magic, 58b1b20 -> f5c35c7 "fixed" some failed tests (They are all using SessionTest or its derived fixture class) that triggered a segmentation fault. I don't think it is a good sign.

I put FileTest.getInfo (one of the failed tests) into gdb on 58b1b20, and found that the trigger is the this pointer I have passed into the constructor of tr_session.global_ip_cache_, it wasn't accessible. And if I put a breakpoint right at the start of the test and step over it, the test finished without a problem.
image

Could you please let me know how do you decide if a member of tr_session would be initialised:

  1. In the constructor's initialization list
  2. By giving it a default initializer
  3. Inside tr_session.initImpl() (Why is it running in a thread?)

libtransmission/session.h Outdated Show resolved Hide resolved
Copy link
Member Author

@tearfur tearfur left a comment

Choose a reason for hiding this comment

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

Add locking mechanisms in these places

libtransmission/net.cc Outdated Show resolved Hide resolved
libtransmission/net.cc Outdated Show resolved Hide resolved
libtransmission/net.cc Outdated Show resolved Hide resolved
libtransmission/net.h Outdated Show resolved Hide resolved
@tearfur tearfur requested a review from ckerr April 18, 2023 15:49
@tearfur
Copy link
Member Author

tearfur commented Apr 23, 2023

@ckerr Ready for review again. Addressed all review comments.

libtransmission/global-ip-cache.h Outdated Show resolved Hide resolved
libtransmission/global-ip-cache.h Outdated Show resolved Hide resolved
libtransmission/global-ip-cache.h Outdated Show resolved Hide resolved
libtransmission/global-ip-cache.h Outdated Show resolved Hide resolved
libtransmission/global-ip-cache.cc Show resolved Hide resolved
libtransmission/session.h Outdated Show resolved Hide resolved
libtransmission/global-ip-cache.cc Outdated Show resolved Hide resolved
libtransmission/session.h Show resolved Hide resolved
libtransmission/global-ip-cache.cc Outdated Show resolved Hide resolved
libtransmission/session.cc Show resolved Hide resolved
@ckerr ckerr merged commit 474a30a into transmission:main May 5, 2023
21 checks passed
@ckerr
Copy link
Member

ckerr commented Jun 11, 2023

@tearfur what is a good way to summarize 5329 / 5498 / 5510 for the 4.1.0 release notes?

@tearfur
Copy link
Member Author

tearfur commented Jun 12, 2023

@ckerr It for sure is hard to boil it down to 1 sentence. 😛

Notes: Added ability to cache IP addresses used in global communications, and use it to fix UDP6 warning log spam.

What do you think? I am wondering if I should directly describe the underlying cause of the UDP6 log spam, or if this is fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:core semver:minor adds functionality in a backwards compatible manner type:feat A new feature
6 participants