Skip to content

Conversation

@iequidoo
Copy link
Collaborator

@iequidoo iequidoo commented Nov 20, 2025

Note that if the server doesn't support IMAP METADATA, the fallback TURN server isn't used. I didn't get why the code is written this way, i.e. create_fallback_ice_servers() is only called if the server supports METADATA, do we want to change this?

Looks working, tested with a nine profile and non-chatmail too.
For testing with the release DC Desktop apply 8f6ce74 also, otherwise the "Call" button isn't even shown.

Close #7382

@iequidoo iequidoo requested a review from r10s November 20, 2025 23:35
@r10s
Copy link
Contributor

r10s commented Nov 21, 2025

Note that if the server doesn't support IMAP METADATA, the fallback TURN server isn't used. I didn't get why the code is written this way, i.e. create_fallback_ice_servers() is only called if the server supports METADATA, do we want to change this?

i agree, this looks wrong.

IMAP METADATA is not supported by all server (iirc, e.g. it is not enabled by default dovecot installs) - and i do not see a good reason why core should not pass default TURN server to UI in that case. esp. as this will result in bad UX and bad debuggability, as we still cannot know a TURN server is returned in all cases.

the only reason that come to mind is that the code gets too complexity, and if IMAP METADATA is supported by >99% of servers - but i doubt both, cc @link2xt will have more insights

@link2xt
Copy link
Collaborator

link2xt commented Nov 21, 2025

do we want to change this?

It's a bug, if IMAP METADATA is not supported we should use the fallback server as well.

@iequidoo iequidoo force-pushed the iequidoo/fallback-turn-server branch from 142b866 to 4ebd5cc Compare November 22, 2025 09:38
@iequidoo
Copy link
Collaborator Author

It's a bug, if IMAP METADATA is not supported we should use the fallback server as well.

Pushed a fix for this as well.

Copy link
Contributor

@Simon-Laux Simon-Laux left a comment

Choose a reason for hiding this comment

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

this pr includes unrelated changes, namely it seems to revert #7285

maybe a mistake that happened during rebase? or sth. left over from development?

@iequidoo iequidoo force-pushed the iequidoo/fallback-turn-server branch from 4ebd5cc to 43609bb Compare November 22, 2025 19:31
@iequidoo
Copy link
Collaborator Author

iequidoo commented Nov 22, 2025

this pr includes unrelated changes, namely it seems to revert #7285

maybe a mistake that happened during rebase? or sth. left over from development?

Removed the reverting commit. I added it initially because w/o it one can't test this PR in the release version of DC Desktop, the "Call" button isn't even shown.

@iequidoo iequidoo requested a review from Simon-Laux November 22, 2025 19:35
@iequidoo iequidoo force-pushed the iequidoo/fallback-turn-server branch from 43609bb to 64b1044 Compare November 22, 2025 23:05
@iequidoo iequidoo force-pushed the iequidoo/fallback-turn-server branch from 64b1044 to 8e6602c Compare November 24, 2025 07:07
Copy link
Contributor

@Simon-Laux Simon-Laux left a comment

Choose a reason for hiding this comment

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

Looks good, but better wait for another review because I didn't took the time to fully understand it.

@iequidoo iequidoo force-pushed the iequidoo/fallback-turn-server branch from 8e6602c to b95812c Compare November 25, 2025 05:48
@iequidoo iequidoo force-pushed the iequidoo/fallback-turn-server branch from b95812c to 22e9e51 Compare December 6, 2025 00:00
@iequidoo
Copy link
Collaborator Author

Finally this looks working for me, i just didn't know how to test it the right way. It's not necessary to force using TURN via the WebRTC API, even having devices in the same network just do smth like:

iptables -A INPUT -s 192.168.1.x -j DROP
iptables -A OUTPUT -d 192.168.1.x -j DROP
ip6tables -A INPUT -s 2800:a4:673:bd00:3e9e:5639:93df:xxxx -j DROP
ip6tables -A OUTPUT -d 2800:a4:673:bd00:3e9e:5639:93df:xxxx -j DROP

Then after an unsuccessful direct connection the offering side will use TURN and the accepting side -- STUN:
Selected candidate pair: local: srflx (STUN) 190.134.202.123 => remote: relay (TURN) 46.224.39.230.

Better if somebody else tests this too, but it's ready for review.

@iequidoo iequidoo requested review from Hocuri and link2xt December 14, 2025 06:28
credential: Some("o4tR7yG4rG2slhXqRUf9zgmHz".to_string()),
};

let json = serde_json::to_string(&[stun_server, turn_server])?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

STUN server can be just removed, if there is TURN server you don't need STUN even to get non-relay candidates. But maybe let's keep it in case we want to shutdown turn.delta.chat for some reason.

Copy link
Collaborator Author

@iequidoo iequidoo Dec 14, 2025

Choose a reason for hiding this comment

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

Yes, let's keep it as is, i tried to remove "nine.testrun.org" fallback STUN, but anyway i see that the accepting side goes to a STUN server of my internet provider (190.134.202.123), so at least for me removal of our fallback STUN changes nothing

@link2xt
Copy link
Collaborator

link2xt commented Dec 14, 2025

cargo-deny CI is fixable with rebase

@iequidoo iequidoo force-pushed the iequidoo/fallback-turn-server branch from 22e9e51 to a179a1a Compare December 14, 2025 19:20
@iequidoo iequidoo merged commit bfa4b6f into main Dec 14, 2025
30 checks passed
@iequidoo iequidoo deleted the iequidoo/fallback-turn-server branch December 14, 2025 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

use fallback TURN server

5 participants