Implement RPC version handshake

[tleedjarv]

As mentioned in the mail sent to unison-hackers list. This patch is intended to introduce a compatibility measure, but while doing so, it is itself a breaking change. Edit: Compatibility with 2.51 is possible to keep.

Background and motivation

With current implementation, when client opens the connection to a server, it checks compatibility by comparing a Unison version string sent by server. This is limiting when it comes to making backwards-compatible changes.

This patch implements a version handshake so that client and server can agree on a common RPC protocol version out of potentially several supported versions.

This is a low-level part of a fix to #407. A separate API-level mechanism is planned for a complete fix to #407.

Description

RPC version is defined as an integer. Versions do not have to be consecutive (or even positive, for that matter), only the value matters. Larger version values are always considered to be more recent than smaller version values. This patch sets the starting version at 1 but it could be any integer.

A release could support several RPC versions (which do not have to be consecutive; could support versions 1 and 3 but not 2, for example). When client establishes a connection to the server, a version is selected which both client and server support, even if it is not the latest version. This allows a client to connect to both older servers (assuming the client supports an older RPC version) and newer servers (assuming the newer server supports an older RPC version).

The version handshake process is described in the code. The handshake process itself is a very simple text-based interaction.

What is it not?

This patch does nothing about incompatibilities caused by different OCaml versions.

[gdt]

Multiple separate comments:

• We are talking about a different serialization protocol. This seems to be a text-based preamble before that, so that future protocols can change formats. That seems good.
• I wonder if version 0 can be defined as starting off with something that fits the current format, so that while looking for the int works, it parses the current as zero. And a switch to force the old one without the int, so that the new code with --format=0 will interop with the old. Basically, I don't see that this has to be breaking.

[tleedjarv]

This seems to be a text-based preamble before that, so that future protocols can change formats.

That's correct.

I wonder if version 0 can be defined as starting off with something that fits the current format, so that while looking for the int works, it parses the current as zero. And a switch to force the old one without the int, so that the new code with --format=0 will interop with the old. Basically, I don't see that this has to be breaking.

True. I even thought about it and at some point decided that it's not worth it. This version handshake itself could rather easily be made non-breaking as you suggest. But the bigger problem is that the actual wire format change that Stéphane is working on would then also have to work alongside the current OCaml-dependent wire format. I don't know how realistic it is to keep supporting the current wire format. Definitely worth considering. I may extend this patch just to have the option, even if we end up not using it.

[gdt]

Good point about how hard it might be to use a non-breaking version -- but if the patch itself is non-breaking, it can be applied sooner, with less angst. None of this is an easy call and I don't pretend to be all wise.

[tleedjarv]

I added what I call 2.51-compatibility mode. The name can be changed of course, this makes no difference to the concept. For the clients, this compatibility mode works automatically. For servers, there is a new switch which makes it serve all clients in 2.51 mode. (When using ssh, this switch must be passed from client with the servercmd preference.)

Changes to the checkHeader function are not showing up very clearly in the diff and I didn't want to touch the existing function too much. The idea is that the new header "Unison RPC" is almost a prefix to old header "Unison 2.51 with OCaml ..." so the checking starts with new header and when that fails, it switches checking to the old header without having to start over from beginning.

[tleedjarv]

I had a look at @glondu's Umarshal implementation and the changes he'd done in the Remote module. I now think that supporting the current and new encoding in parallel is very feasible and could be set as an explicit goal. I've added a commit to make it as straightforward as possible to add Umarshal encoding functions in Remote and keep it backwards-compatibile at the same time.

[tleedjarv]

Rebased and refactored. Code is slightly longer now but hopefully somewhat easier to read.

New: Added an extension mechanism to the handshake protocol.
This allows implementing optional handshake extensions (plugins) that are RPC version dependent or indepentent and possibly controlled by user preferences.

Handshake extensions are intended for low-level needs that could benefit from being executed before the full RPC mechanism is initialized. Extensions are always initiated from server side.

One example of such handshake extension is authentication. Another example is version-independent negotiation of RPC encoding format (RPC version x is available both in binary format and JSON, for example).

It is not required to merge the extensions mechanism before any actual extensions exist. It may also very well change between RPC versions now that version negotiation is the very first thing done.

Version compatibility with this PR

		Server
		2.51	This PR
Client	2.51	OK, native	Preference on server
	This PR	OK, automatically	OK, native

[tleedjarv]

Added a mechanism for server to automatically switch between the 2.51-compatibility mode and the new versioned RPC mode when using ssh. It only works over ssh and not with socket because with ssh the server is started by client, so client can pass extra information to server.

Version compatibility with this PR

		Server
		2.51	This PR
Client	2.51	OK, native	ssh: OK, automatically socket: OK, manual preference on server
	This PR	OK, automatically	OK, native

[gdt]

(Now that the release is done I'm coming back around to PRs that were obviously post-release.)

It seems this is a break if using socket mode and the old pref isn't configured. I wonder about the wisdom of making servers do the old way unless configured to be new. I also wonder if socket mode should be deprecated and even removed; it seems like a security fail, and ssh, or some other kind of wrapper (TLS?) seems like the way to go. I'll float that on users.

I also wonder about the extensions scheme; if that can be safely deferred I tend to want to defer it, as it feels speculative, and I'd like to avoid second system syndrome.

[avollmerhaus]

I'll float that on users.

Speaking only for myself (obviously), in my over 10 years of daily unison usage I have never used the socket mode and won't miss it.
That said, I'm very much looking forward to the RPC version handshake and consider it to be one of the best things to happen in unison in a long time.

[tleedjarv]

Agree about the extensions scheme. For a moment there I had a thought that this was the only opportunity to prepare for it. That is obviously false, so I've removed that commit.

Now added complete 2.51 compatibility. It relies on tricks with the old protocol. See code comments for the details.

Version compatibility with this PR

		Server
		2.51	This PR
Client	2.51	✔ OK, native	✔ OK, automatically
	This PR	✔ OK, automatically	✔ OK, native

[tleedjarv]

I added two commits with preparation work for keeping full 2.51 compatibility with future type changes. With this preparation, I can make the ACL and xattr patches completely non-breaking, for example.

These two are not strictly necessary to merge with this PR but since they depend on the other commits then I chose to add them here for review instead of opening a separate PR.

There are design choices and assumptions that I'll describe more in the hackers list.

[tleedjarv]

One more commit to make for a smoother upgrade from any version that uses archive format 22 (interestingly, this seems to be all versions back to at least 2.13 -- the archive format version has not changed during almost 17 years of history visible on GH). Note that this is just about upgrading, it is not related to client-server version differences.

This, like previous commits, is just the foundation. The archive format has not changed yet. The ability to upgrade does not depend on why the archive format will change -- either because of switching from Marshal to Umarshal or because of type changes.

Like previous two commits, this one is not strictly necessary to merge with this PR but it makes sense to keep all the commits together as they form a solid foundation on which to base the Umarshal work. Once Umarshal is rebased on these commits, the switch to it will be transparent (or so we hope).

[glondu]

I've reviewed until 651dd13, and except for the enableFlowControl function it looks good.

[glondu]

The calls to Lwt.ignore_result look suspicious here, especially the first one. Are you sure you don't want to wait for flushBuffer to be complete before setting q.flowControl to true? Given the contexts in which this function is used, I would turn it into a proper monadic function.

[tleedjarv]

I pretty much followed the example of allowWrites and disableFlowControl from the original code (... except, that doesn't explain the flushBuffer -- that's apparently all me). I've now changed it as you proposed (and fixed a bug I found while at it).

[gdt]

I reread all the comments, and it seems like we are in a state with no other mergeable PRs, so I think it's time to seriously consider merging this. If you concur, a rebase might be nice because it should be trivial, just to clean up history, perhaps take something off the table, and get CI artifacts with other recent changes. Then perhaps time to ask for more review, comments on the wisdom of merging, and testing. If you don't think it would be wise to merge now, please add a comment explaining why.

[tleedjarv]

I have rebased and added some temporary patches for compatibility with older compilers (I will create a separate PR to remove all such patches after the next release).

[gdt]

Merge scheduled for 15 14Z January.