Support TCP for protocol messages#3636
Conversation
softins
force-pushed
the
tcp-protocol
branch
4 times, most recently
from
5e1a658 to
0ae51e2
Compare
softins
force-pushed
the
tcp-protocol
branch
3 times, most recently
from
7ad1d1f to
d939e5b
Compare
|
So the next stage of implementation has been achieved: client-side support in the Connect dialog.
It has been tested by using Examples for a directory-enabled server running on port 22120:
Note that
|
|
The next step is to try implementing the connected-mode TCP described here |
| bool bUseTranslation = true; | ||
| bool bCustomPortNumberGiven = false; | ||
| bool bEnableIPv6 = false; | ||
| bool bEnableTcp = false; |
There was a problem hiding this comment.
Since we'll have a long time for the 4.0 release, I'd enable it by default soon (of course once we've tested that the basics work)
There was a problem hiding this comment.
No, I disagree. It's a server-only option, and most servers operators will not need to enable TCP support. Only those running large directories or large servers will need to, and they also need to understand and configure their firewall requirements.
TCP support in the client will indeed be enabled by default, but will only take effect when talking to a directory or server that has enabled it.
If a server operator enables TCP without having configured their firewall correctly, client users could have problems as the server would advertise TCP support to the client, but the client could be unable to connect.
There was a problem hiding this comment.
Can we not give an error message or fallback procedure in case the TCP connection timed out?
There was a problem hiding this comment.
Yes, I'm sure we can. I haven't yet tested that scenario.
But it doesn't negate my view that server-side TCP support needs to be an explicit option.
There was a problem hiding this comment.
|
Well I've finished implementing everything I intended to, for directory, server and client, so it's ready for reviewing and trying out, as and when time permits (post 3.12.0). I have a private directory and server built and running with TCP support, at In order to demonstrate the use of TCP in a new client's connect dialog, it will be necessary to use custom firewall filters on the client end to temporarily drop incoming UDP Jamulus protocol messages containing a server list or connected clients list. There is full forward and backward compatibility between clients and servers built with TCP support and older versions. |
|
Keeping as draft, because it will need quite a few debug messages removed before merging. |
|
|
||
| If a server were to offer TCP to the client, but the server's firewall didn't allow the incoming TCP connection, the client request for TCP would wait until its request times out. | ||
|
|
||
| This has to be the responsibility of the server/directory operator, and is why TCP operation must be controlled by a command-line option, rather than always enabled. The operator should only enable TCP in the Jamulus server if they know their environment has been configured to support it. |
There was a problem hiding this comment.
I tend to disagree with it being a reason for not always enabling it, as stated. Things should carry on working exactly as they would without it -- except an extra exchange happens that fails. Enabling by default IPv6 does the same.
|
|
||
| This has to be the responsibility of the server/directory operator, and is why TCP operation must be controlled by a command-line option, rather than always enabled. The operator should only enable TCP in the Jamulus server if they know their environment has been configured to support it. | ||
|
|
||
| Most operators of small servers of directories will not need to be concerned with TCP at all. _The only server operators who will need to enable TCP support are those running large directories (e.g. Volker, Peter) or those running a large server designed to support many simultaneous client connections._ |
There was a problem hiding this comment.
This bit is the real reason. I think it should be explain like this primarily, rather than on the technicalities.
It's pointless doing it for everyone. It doesn't matter, it's just pointless.
|
|
||
| The reason for using a TCP connection in an active session is just to provide a reliable path for delivering a list of connected clients that could be large and subject to fragmentation (if it is sent over UDP). So the established TCP connection is only used to deliver client lists, and not other protocol messages. | ||
|
|
||
| Therefore, if the server has an active TCP connection from the client, it will use the connectionless `CLM_CONN_CLIENTS_LIST` message to deliver updates for the connected client list. If there is no active TCP connection, updates will be delivered using the connected-mode `CONN_CLIENTS_LIST` over UDP as at present. |
There was a problem hiding this comment.
Might be worth noting at this point (as it's in the flow and notable) why the two messages are different - i.e. why it doesn't bother to use the CONN_CLIENTS_LIST.
There was a problem hiding this comment.
OK, I'll add something. The reason is that CONN_CLIENTS_LIST messages are numbered and acked, to provide reliable delivery or retry over UDP. That's not needed over reliable TCP and would complicate things, so a simple CLM_CONN_CLIENTS_LIST is used instead.
|
I have a few things to change here:
|
softins
force-pushed
the
tcp-protocol
branch
2 times, most recently
from
c78767e to
1ac1a02
Compare
|
Probably worth squashing now. How close do you reckon it is to safe to use on a private Directory and Server for testing? It really would be good to write up a set of test cases, with the logging enabled to show it's working right. Things like explaining how to test over a LAN... (I'd not have a clue.) |
Well I have an outstanding significant structural change to make on the client side, but am on holiday for two weeks from tomorrow, and I don't know if I will get time to dip into it while away. It certainly should work in its current state, but maybe better to wait until I'm back in circulation. |
|
In the meantime, if anyone wants to test, you can set up a server and a client on the same LAN, or even the same machine. The server should also be a directory, using |
|
As mentioned in an earlier comment, I have a directory at |
|
I have reworked the client side to do all the UDP/TCP selection at the Instead of storing the UDP/TCP mode for a particular server in the connect dialog, it now uses a I've also squashed the multitude of commits into a single commit (although there is now a second to follow it). I still need to remove or comment out a lot of debug messages which were intended more for proving to myself the logic during development, than they were for production. |
| QTcpSocket* pSocket = new QTcpSocket ( this ); | ||
|
|
||
| // timer for TCP connect timeout because Qt defaults to 30 seconds | ||
| // and we want it to be 3 seconds (TCP_CONNECT_TIMEOUT_MS) |
There was a problem hiding this comment.
This comment needs clarifying: nowhere in the close neighbourhood is TCP_CONNECT_TIMEOUT_MS mentioned. (I can see it's kicked off after all the set up bits have set things up - maybe just mention that.)
|
Very nice. It makes it more like "here's another way the to handle protocol messages". |
| { | ||
| qDebug() << "- sending client list via TCP"; | ||
|
|
||
| ConnLessProtocol.CreateCLConnClientsListMes ( InetAddr, vecChanInfo, pTcpConnection ); |
There was a problem hiding this comment.
I think we should make clear why the connectionless protocol is used here - I suppose since TCP handles the session, it's enough.
| switch ( eFetchMode ) | ||
| { | ||
| case CFM_UDP_REQUEST: | ||
| qWarning() << "Unsatisfied Client List request via UDP for" << InetAddr.toString(); |
There was a problem hiding this comment.
| qWarning() << "Unsatisfied Client List request via UDP for" << InetAddr.toString(); | |
| qWarning() << "Unsatisfied Client List request via UDP for " << InetAddr.toString(); |
There was a problem hiding this comment.
This isn't needed. qWarning() and its relatives automatically put a space between << items.
| ConnLessProtocol.CreateCLReqConnClientsListMes ( InetAddr, PROTO_UDP ); | ||
| break; | ||
| case CFM_TCP_REQUEST: | ||
| qWarning() << "Unsatisfied Client List request via TCP for" << InetAddr.toString() << "(switching back to UDP)"; |
There was a problem hiding this comment.
| qWarning() << "Unsatisfied Client List request via TCP for" << InetAddr.toString() << "(switching back to UDP)"; | |
| qWarning() << "Unsatisfied Client List request via TCP for " << InetAddr.toString() << "(switching back to UDP)"; |
| { | ||
| if ( pendingClientList.value ( InetAddr ) == CFM_UDP_REQUEST ) | ||
| { | ||
| qDebug() << "- UDP client list not received from" << InetAddr.toString() << "- retrying via TCP"; |
There was a problem hiding this comment.
We might want to log this in production.
There was a problem hiding this comment.
Possibly. I was actually considering removing it, as it can be quite noisy if a directory contains servers that are either not configured properly or very distant.
| - PROTMESSID_CLM_CLIENT_ID | ||
|
|
||
|
|
||
| - PROTMESSID_CLM_CLIENT_ID: Sends the client's channel ID back to the server |
There was a problem hiding this comment.
Any security implications if the client sends a wrong id?
There was a problem hiding this comment.
Not that I can think of, because I made sure the server will only accept it if it comes from the correct IP address. Otherwise it's ignored.
| } | ||
|
|
||
| bool CProtocol::EvaluateCLReqConnClientsListMes ( const CHostAddress& InetAddr ) | ||
| bool CProtocol::EvaluateCLReqConnClientsListMes ( const CHostAddress& InetAddr, CTcpConnection* pTcpConnection ) |
There was a problem hiding this comment.
Any reason why we need to pass CTcpConnection that often and cannot just pass it once to the class?
There was a problem hiding this comment.
Because we have a separate TCP connection with each different peer.
| { | ||
| // send empty message to keep NAT port open at registered server | ||
| pConnLessProtocol->CreateCLEmptyMes ( ServerList[iIdx].HostAddr ); | ||
| pConnLessProtocol->CreateCLEmptyMes ( ServerList[iIdx].HostAddr, nullptr ); |
There was a problem hiding this comment.
CreateCLEmptyMes could also have a default argument == nullptr I suppose.
Then we should probably rename it to express that it is not just TCP support but rather that it request a TCP connection due to network problems. |
But it doesn't mean that. The message is still sent and received if there are no network problems. It is the client that decides what to do with it, by inferring network problems only if |
|
It's more "TCP is available as an alternative to the UDP response you were expecting (<this one>) - ask and you'll get it reliably". |
3 participants
Short description of changes
Support fallback to TCP for protocol messages, in order to overcome potential loss of large messages due to UDP fragmentation.
Currently an incomplete draft, for comment as development continues.CHANGELOG: Client/Server: Support TCP fallback for protocol messages.
Context: Fixes an issue?
Discussed in issue #3242.
Does this change need documentation? What needs to be documented and how?
It will need documentation once design and development are complete. Particularly need to explain the firewall requirements for a server or directory.
Status of this Pull Request
Incomplete, still under development. Main server side complete and working. Client side development in progress.Complete and ready for review and testing.Still marked draft asit needs some of the debug messages to be commented out before merging.What is missing until this pull request can be merged?
A lot of testing of both server and client. Intended for Jamulus 4.0.0.
Checklist