Syncplay / syncplay

Client/server to synchronize media playback on mpv/VLC/MPC-HC/MPC-BE on many computers
http://syncplay.pl/
Apache License 2.0
2.16k stars 216 forks source link

IPv6 Support #107

Closed dawnMachine closed 5 years ago

dawnMachine commented 8 years ago

Just wanted to leave a suggestion for IPv6 support. I was in the IRC for a somewhat related issue, and someone there told me I should create an issue here. They also said that it should already have it, but when I tried to put in my IPv6, it read everything after the first colon as the port. (Since IPv6 format is abcd:123:12cd:ab34:1b3d:a2c4:a23d:1bc4, using colons.) Just a suggestion, as finding my IPv4 was a pain in the behind, and more and more ISPs are handing out IPv6s nowadays. If it does already support it, could you update the guide with how to use it? Because the obvious way doesn't work. ^^;

Thanks for your time!

nilsding commented 8 years ago

Have you tried putting the address in brackets? (e.g. [abcd:123:12cd:ab34:1b3d:a2c4:a23d:1bc4]:8880)

Uriziel commented 8 years ago

@nilsding It won't work as Syncplay server's interface doesn't listen on IPv6.

ghost commented 8 years ago

Hi,

the provided patch should allow you to use IPv6

diff --git a/syncplay/messages.py b/syncplay/messages.py
index 7abe1ad..25121ef 100755
--- a/syncplay/messages.py
+++ b/syncplay/messages.py
@@ -360,6 +360,7 @@ en = {
       "server-motd-argument": "path to file from which motd will be fetched",
       "server-messed-up-motd-unescaped-placeholders": "Message of the Day has unescaped placeholders. All $ signs should be doubled ($$).",
       "server-messed-up-motd-too-long": "Message of the Day is too long - maximum of {} chars, {} given.",
+      "enable-ipv6": "enable ipv6 support?",

       # Server errors
       "unknown-command-server-error" : "Unknown command {}",  # message
diff --git a/syncplay/server.py b/syncplay/server.py
index 3996311..ca336ee 100644
--- a/syncplay/server.py
+++ b/syncplay/server.py
@@ -443,3 +443,4 @@ class ConfigurationGetter(object):
         self._argparser.add_argument('--disable-ready', action='store_true', help=getMessage("server-disable-ready-argument"))
         self._argparser.add_argument('--salt', metavar='salt', type=str, nargs='?', help=getMessage("server-salt-argument"))
         self._argparser.add_argument('--motd-file', metavar='file', type=str, nargs='?', help=getMessage("server-motd-argument"))
+        self._argparser.add_argument('--ipv6', action='store_true', help=getMessage("enable-ipv6"))
diff --git a/syncplay/ui/ConfigurationGetter.py b/syncplay/ui/ConfigurationGetter.py
index 0fa6284..51a64c4 100755
--- a/syncplay/ui/ConfigurationGetter.py
+++ b/syncplay/ui/ConfigurationGetter.py
@@ -219,7 +219,7 @@ class ConfigurationGetter(object):
         port = constants.DEFAULT_PORT if not self._config["port"] else self._config["port"]
         if host:
             if ':' in host:
-                host, port = host.split(':', 1)
+                host, port = host.rsplit(':', 1)
                 try:
                     port = int(port)
                 except ValueError:
diff --git a/syncplayServer.py b/syncplayServer.py
index b4456d7..a367e19 100755
--- a/syncplayServer.py
+++ b/syncplayServer.py
@@ -20,5 +20,11 @@ if __name__ == '__main__':
     argsGetter = ConfigurationGetter()
     args = argsGetter.getConfiguration()

-    reactor.listenTCP(int(args.port), SyncFactory(args.password, args.motd_file, args.isolate_rooms, args.salt, args.disable_ready))
+    factory = SyncFactory(args.password, args.motd_file, args.isolate_rooms, args.salt, args.disable_ready)
+
+    if args.ipv6:
+        reactor.listenTCP(int(args.port), factory, interface='::')
+    else:
+        reactor.listenTCP(int(args.port), factory)
+
     reactor.run()
Uriziel commented 8 years ago

It does, but it means either IPv6 or IPv4 which is not ideal.

schoerg commented 7 years ago

@Uriziel On Linux interface="::" will allow both IPv4 and IPv6 clients to connect.

tcp6       0      0 :::8999                 :::*                    LISTEN      5805/python2
tcp6       0      0 192.168.0.6:8999        192.168.0.10:12527      ESTABLISHED 5805/python2

Can't test with IPv6, as the client doesn't like the format.

As far as I know, Windows doesn't do that by default: https://serverfault.com/questions/21657/semantics-of-and-0-0-0-0-in-dual-stack-oses?answertab=active#tab-top

Mikaela commented 6 years ago

Are there any news about this? I would like IPv6 support as at the moment my client is behind CGN sometimes causing connection issues and my server is a cheap NAT VPS with only 20 open ports IPv4.

I am also using Cjdns and Yggdrasil-network that are IPv6-only mesh/overlay/mixnetworks.

I cannot use the patch above as at least one party requires IPv4 and that :: being dualstack depends on kernel configuration, I am sure which it defaults to on different distributions, but sometimes I disable it by myself to avoid mapped addresses.

Mikaela commented 5 years ago

@CcxCZ is working on this at https://github.com/Syncplay/syncplay/compare/master...ccxcz:endpoints, but has problems with having time, so he hasn't opened a PR yet.

If you would like to try it, I have a server running it --host ds.relpda.mikaela.info:14404 --password ccxcz-endpoints-demo.

The server side command is python3 /opt/syncplay/syncplayServer.py --endpoint tcp:14404 --endpoint tcp6:14404 --motd-file /opt/syncplay/motd.txt --salt <CENSORED> --password ccxcz-endpoints-demo and I have net.ipv6.bindv6only as 1 so :: won't accept IPv4 connections with dotted decimals. I think there was some very old Twisted issue on that not being the default, but I tend to set it to kernel by myself.

Et0h commented 5 years ago

@albertosottile has also done some work on IPv6 at https://github.com/albertosottile/syncplay/tree/IPv6

albertosottile commented 5 years ago

I apologize if I did not comment before, but I just moved to a new country and did not have much free time lately.

The code in my IPv6 branch (https://github.com/albertosottile/syncplay/tree/IPv6) should fully support IPv6, with the nice feature of having a single server that supports both IPv4 and IPv6 clients in the same room. I do not know if this is possible with @ccxcz code given that the patch uses multiple endpoints for the server (I like the idea of returning the Deferred() for catching debug info, though).

The patch was "finished" weeks ago, but unfortunately, I could not (and still cannot) test it properly because my ISP and my LAN do not support IPv6 at all. I made some preliminary tests using VPSs and everything seemed to work fine but, of course, further tests are needed before releasing it.

@Mikaela if you could run the server and the client from that branch, and provide some feedback to us, that would be great. I understand that you have a peculiar network setting, though (net.ipv6.bindv6only = 1), which could prevent coexistence of IPv4 and IPv6 clients on your test server with the current code. We'll see about it if you manage to get the server running.

Mikaela commented 5 years ago

With Ccx's branch I am using IPv6 (I am behind CGN) and he is using IPv4 (so there is no problem with mixed IP clients, we havn been using this setup for a long time (what is the time in those commits?)). I have specified the endpoints separately in the server side for that sysctl option to avoid https://serverfault.com/questions/408667/how-do-i-disable-ipv4-mapped-ipv6 , without it I would get error about port already being in use. I understood that his code also supports Unix sockets, but we didn't test that.

I fear that I cannot give promises on testing, but the easiest way to get IPv6 is installing Miredo on Linux or enabling the inbuild client on Windows, even if there are a lot better transition methods.

Mikaela commented 5 years ago

@albertosottile Do I need to do something special on the server or the client with your branch? Edit: Like with Ccx's branch I have to specify --endpoint tcp:14404 --endpoint tcp6:14404 in order to have it listen dualstack, (because of net.ipv6.bindv6only = 1)?

I added the remote on both, fetched it, and checkouted to IPv6, then ran the server with

 /opt/syncplay/syncplayServer.py --port 14404 --motd-file /opt/syncplay/motd.txt --salt REDACTED --password demo

and attempted to connect to y.relpda.mikaela.info:14404 (Yggdrasil) and relpda.mikaela.info:14404 (IPv6-only) and I kept receiving "Error connecting with the server" or similar message.

Everything started working again on returning to Ccx's branch.

Et0h commented 5 years ago

What happens if you put the hostname in square brackets or use an IPv6 IP address?

ccxcz commented 5 years ago

So this patch was a really quick job that had two goals: get something working so it can be used and tested, and to showcase the serverFromString/clientFromString API so it can be discussed whether it's worth making a breaking change to configuration or not and if so then how.

Twisted provides plugin mechanism so anyone can easily add things that can be listened on or connected to. In the Twisted core you get support for TCP over IPv4 and IPv6, Unix sockets (handy for Tor or similar), TLS, inherited filedescriptors (handy for UCSPI), SystemD socket activation, and possibly more in the non-core parts of Twisted. I know there is SSH client endpoint there, probably some SOCKS and WebSocket implementations floating around... in general pretty much whatever one could want. The downside is that the syntax is unfamiliar and sparsely documented. And as mentioned above it would be a breaking change to how Syncplay is configured. You can support both of course, but that can get confusing too.

I used --endpoint option both in client and server. In retrospect --connect and --listen respectively would be way more intuitive names. Server can listen on any number of different endpoints, with the caveat of there not being code to explicitly disable 6 to 4 mapping if OS does that by default as pointed above, other than that you can mix and match what it will listen on as you need. Client obviously uses just one endpoint to connect to.

What surprised me when writing the patch was that there was absolutely no error handling for those very obviously fatal errors. Adding it to server as easy as the logic was simple, but the client is tangled mess of many possible event loops and UI frameworks, so I gave up on adding that as a trivial change. Deferreds are the the Twisted way to write asynchronous exception handling, so using them here is only natural. They compose well when you have multiple conditions to check, either DeferredList with it's many option combination or just d1.addErrback(d2.errback) as I did in the server and it will propagate with right message and traceback. For GUI clients I usually end up writing analogs of the react() function that install the right reactor, instantiate needed globals (eg. QApplication) and then call the main function which doesn't need to care about setting up Twisted the right way anymore. I'm not sure what the best approach would be here, as I haven't looked at the code deep enough, but I believe something similar could simplify the matters quite a bit.

Mikaela commented 5 years ago

Is there any timeline for official IPv6 support? I just noticed that 1.6.2 was released and I thught about this.

/opt/syncplay/syncplayServer.py --port 14404 --motd-file /opt/syncplay/motd.txt --salt REDACTED --password demo

What happens if you put the hostname in square brackets or use an IPv6 IP address?

Sorry, I still haven't had the time to test this, but I hope it's not hanging on me as no one replied to ccxcz above either.

Et0h commented 5 years ago

Reliable IPv6 support would be great. I am not super fond of breaking changes if they can be avoided, so I would need a strong case for why one would be desirable.

I am on IPv4 so am only of limited assistance in moving things forwards, but in terms of what I'd ideally want to see is something which changes Syncplay as little as possible to achieve the end result. This is to: (a) reduce the likelihood of the change introducing unexpected problems which are hard to track down; (b) help maintain forwards and backwards compatibility; and (c) avoid the need to update guidance and respond to queries from users who do not know why things stopped working how they expected.

I do not know why we need new client command line switches as included in the @ccxcz code - maybe there is a great rationale, if so I would need to hear it. In terms of what @albertosottile proposes I've heard reports from people that it doesn't work (i.e. the post from @Mikaela Mikaela) but no reports from anyone that it does work - as such, for now I'm waiting for someone who knows more about IPv6 than me to help move things forwards (e.g. by confirming issues, proposing fixes, and/or saying it actually works for them).

Mikaela commented 5 years ago

TL;DR status update from albertosottile so it won't get lost to IRC:

TL;DR tests this morning showed that the IPv6 branch works. However, there is a Deferred error that must be correctly caught before merging it in master. Everything is extensively detailed here https://github.com/albertosottile/syncplay/commit/d1d0a661164213aea4c8df5c0039c13449954992 so we won't forget

albertosottile commented 5 years ago

Sidenote to avoid forgetting what we did today: in the current implementation, by default the server always accepts IPv6 connections (technical: it uses twisted.internet.endpoints.TCP6ServerEndpoint). It accepts also IPv4 connections if the OS is set accordingly (e.g. net.ipv6.bindv6only = 0). This is the default behavior in most configurations, but it can be broken if the user changes this setting on purpose.

Reversing this behavior is not possible using Twisted. However, it should be possible to create a socket that listens to both IPv4 and IPv6 interfaces overriding the OS settings (e.g. by setting setsockopt(socket.IPPROTO_IPV6, socket.IPV6_V6ONLY, 0) explicitly) but so far I did not find a way to access the socket properties from the Twisted endpoints interface. We asked for more info on the #twisted-dev IRC channel and we are waiting for an answer. Alternatively, I could open an issue on their repository once I have a little more spare time.

EDIT: "fixed" in https://github.com/albertosottile/syncplay/commit/eeb0138d73768c5c7981da3e01fcd8a2233b8c96 by reverting the server to listenTCP and overriding the createInternetSocket method to act on the socket directly.

Et0h commented 5 years ago

@albertosottile I've made an initial attempt at fixing the connection error handling in your branch: http://syncplay.pl/Fix_connection_error_handling.patch - this is based on the guidance at https://twistedmatrix.com/documents/16.4.1/core/howto/endpoints.html#persistent-client-connections

albertosottile commented 5 years ago

Closed by PR #215.