Opened 4 years ago

Last modified 6 months ago

#16535 new patch

less delay for xmpp file transfer using streamhosts

Reported by: evert_mouw Owned by:
Milestone: Patches Needing Review Component: libpurple
Version: 2.10.11 Keywords: XMPP streamhost filetransfer
Cc:

Description

A streamhost is used in xmpp (jabber) as a proxy to stream file contents.

This patch solves a few issues raised in ticket #14390

  • do not try 0.0.0.0 as streamhost
  • be a little bit smarter about the order of the streamhosts being tried
  • shorten the delay for all cases

Although the OS should not offer 0.0.0.0 in the first place, this behavior is still the case and end user experience is affected. Filtering out 0.0.0.0 is only one line of code.

I have decreased the STREAMHOST_CONNECT_TIMEOUT from 15 to 5 seconds. We IMHO don't want to extensively try to get a connection with a slow-responding proxy. We definitely don't want Pidgin to crash due to long timeouts.

Also I tried to be just a little bit smarter about the order of streamhosts being tried.

  • hostnames starting with a letter (not an integer) are tried earlier than hostnames that consists just of an IP address
  • streamhosts using port 7777 are very likely proxy65 daemons specially configured to act as a streamhost, so they also are tried a bit earlier

I've tested my code on my Arch Linux machine (current, 64 bit) and when my girlfriend from another location tries to send me a file, I no longer have to wait 30 seconds. The transfer starts immediately. Note that I have a proxy65 installed and configured on another machine, DNS resolvable, with port number 7777.

Cheers, Evert Mouw <post@…>

Attachments (2)

si.c.patch (1.5 KB) - added by evert_mouw 4 years ago.
does smarter streamhost selection and decreases the timeout
si.c.2.patch (1.3 KB) - added by evert_mouw 4 years ago.
patch that sets timeout from 15 to 10 and discards 0.0.0.0 as streamhost

Download all attachments as: .zip

Change History (17)

Changed 4 years ago by evert_mouw

does smarter streamhost selection and decreases the timeout

comment:1 Changed 4 years ago by Robby

  • Milestone set to Patches Needing Review

comment:2 Changed 4 years ago by datallah

Avoiding using 0.0.0.0 is certainly sane, the shorter connect timeout is *probably* reasonable.

The other changes to the ordering don't seem right. What makes a DNS hostname better than an IP address? Why would I want to use a proxy instead of a direct connect first?

comment:3 Changed 4 years ago by evert_mouw

Hi datallah,

The ordering as it is currently done is, as far as I understand, completely random, so any ordering that has a few assumptions right is an improvement.

My first assumption is that direct connections between XMPP clients very often fail due to NAT and firewalls. If there is a proper proxy proposed by the client (it does not propose a proxy without reason! the user did put the proxy in the configuration with the intention to actually use that proxy), then use that! My code just tries to order the streamhosts offered by the peer (client).

My assumption is that hosts with DNS hostnames have a higher chance of being a properly configured proxy. If they are on port 7777, then it is almost sure that it is a well-configured proxy.

If the patch would be accepted without the ordering, but with 0.0.0.0 filtering and shorter timeout accepted, it would already be an improvement. But please give the ordering some thought. Especially the "if port 7777, assume higher probability of being the best shot" is useful, IMHO.

EDIT: two more comments

  • The whole ordering guessing stuff was done because in the XMPP stanza, both the peer itself and the proxy offered by the peer are offered as streamhosts, so I could not identify the proxy.
  • In a perfect world, I would do parallel connection attempts to all streamhosts offered, and use the fastest one.
Last edited 4 years ago by evert_mouw (previous) (diff)

comment:4 Changed 4 years ago by datallah

The current ordering isn't arbitrary, it uses the ordering specified by the sender (which is what is specified by XEP-0065). At least when Pidgin is the sender (and presumably for other clients too), the ordering is intentional and prefers direct connections over proxies - see jabber_si_xfer_bytestreams_listen_cb in the same file.

It's certainly true that direct connections are less likely to succeed, but switching the ordering will make large in-network file transfers not possible.

A somewhat more complicated algorithm probably could be used to decide whether or not to try internal IPs (and those could likely have a much shorter timeout than public IPs).

comment:5 Changed 4 years ago by evert_mouw

Ah, I was not aware of that... if the specs say that we should use the ordering specified by the sender, then we should honor that. Should I upload a new patch with the offending code removed?

This begs a new question: why is Pidgin offering such streamhosts? It should offer the proxy first, I think, if the user specified a proxy in the account options. That might require a new ticket, I know.

comment:6 Changed 4 years ago by datallah

I don't think it's obvious that we should offer the proxy first.

In a corporate setting where direct connections are likely to work, the current behavior is optimal.

It's actually problematic that we have a default file transfer proxy, we probably shouldn't do that because it's not obvious that the file will be routed through an arbitrary server on the internet.

comment:7 Changed 4 years ago by evert_mouw

Yes, the default file transfer proxy is a security/privacy problem. A sane default would be "no proxy". But if a user specifies a custom proxy, why not honour that?

Or maybe only offer the custom proxy when the peer is not on the same network.

comment:8 Changed 4 years ago by datallah

The sender can't tell if the recipient is on the same network.

The recipient can't even tell if the sender's private IP is useable without trying, but we can make the timeout much faster for private IPs.

comment:9 Changed 4 years ago by evert_mouw

So,

  • sender should not have a default proxy configured
  • receiver should timeout on private IPs faster
  • shorter timeout in general
  • don't even try 0.0.0.0

?

comment:10 Changed 4 years ago by datallah

"shorter timeout in general" is the only questionable thing - we probably can reduce it slightly, but I wouldn't go under 10 seconds.

For private IPs, maybe 5 is acceptable - it seems a bit controversial though.

comment:11 Changed 4 years ago by evert_mouw

Back to geany then... I will submit a new patch. But first, I've to get some sleep ;)

comment:12 Changed 4 years ago by mmcco

Just removed the default file transfer proxy:

https://hg.pidgin.im/pidgin/main/rev/5813479c8e02

comment:13 Changed 4 years ago by mmcco

Note that the default proxy also gets retroactively removed from the user's preferences. This may cause a few support tickets but because it's a fallback option only a few people should be affected.

Changed 4 years ago by evert_mouw

patch that sets timeout from 15 to 10 and discards 0.0.0.0 as streamhost

comment:14 Changed 10 months ago by evert_mouw

Guys, this is 3+ years ago... the patch works for me, please test/accept it :-)

comment:15 Changed 6 months ago by QuLogic

Sorry this seems to have fallen off the radar. Can you open a PR on BitBucket with the change? It will be reviewed much faster there.

Note: See TracTickets for help on using tickets.
All information, including names and email addresses, entered onto this website or sent to mailing lists affiliated with this website will be public. Do not post confidential information, especially passwords!