Allow the client to use the server time

[svanharmelen]

In some situations, were the server setup is managed by a 3rd party, it can be needed to “follow” the server time in the client in order to be able to setup a connection.

This solution is pretty much inline with what is documented here, except in this solution only the time used in the client is adjusted and the host clock is left untouched.

And just to be clear, the offset is only used for the communication and security related parts between the client and the server. For the rest the actuall time will still be used.

I tested this against several 3rd party server and the change resulted in stable connections without any errors, reconnects or constantly renewing security channels.

Fixes #89

[svanharmelen]

I thought of another solution for this problem that I want to try out... I expect it will make the diff a bit bigger, but I think it will be a more robust solution that solves an additional problem.

Will let you know if it works out and when the PR is ready to be reviewed.

[locka99]

okay thanks - I did glance over the first patch and realised it would take more concentration than I had at the time to see what it was doing

[svanharmelen]

@locka99 I think it's a good time to get your input on this one...

I kept the initial commit and added two new commits. The first one is a very big commit, but it essentially only does a rename. So probably best to focus on the other two commits.

The initial commit introduces a solution to allow the client to connect to a server who's clock isn't synced. On itself the solution works perfectly and allows you to connect without continuous reconnects or other issues. But it only focuses on the session itself, not on the actual data being send.

So the last commit updates the solution so it applies the offset to all timestamps that are decoded by the client (except for the source timestamp for obvious reasons). The big advantage is that it means that all data will use the correct time as known by the client. So this assumes the client is configured correctly and that the server is outside of your control.

Now I did look at renaming DecodingOptions to EncoderOptions and also passing it to all encode methods. But that turned out to be an even bigger change and I wasn't sure if that was actually needed (I don't know exactly what timestamps the client sends to the server except for the timestamp in the headers).

So while it "feels" like an even more robust solution (the offset is then applied both ways during encoding and decoding which means all timestamps are consistent and you don't have to think about any exceptions in the code), I wasn't sure if it was worth the extra code and logic needed to pass the EncoderOptions around to every location that calls the encode method. I also looked at making the encode method accept Option<EncoderOptions> to make it easier, but that was also the point that I thought to first discuss all this with you 😏

Very open to add the missing logic to make the move to EncoderOptions if you want to go that route, but curious what you think of all this and if the current solution is maybe enough to support the ability for clients to be able to work with servers with time drift.

Also open for any kind of video call if that would make this conversation easier. Just let me know if you prefer that 😉

[locka99]

Would you be able to rename DecodingOptions back to DecodingLimits and decoding_options to decoding_limits? I think it would shrink the size of the diff down and make it easier what has changed. We can do that separately if you think it's necessary.

[svanharmelen]

Yeah I can revert that, give me 10 minutes 😉

[svanharmelen]

Make that 25 minutes... Got distracted a bit 😉 But I refactored things a bit so it only shows the functional changes. I do have the other commits stored in another branch, so maybe we want to revert it back after your review? Of indeed make a dedicated PR for it once this one has landed...

[svanharmelen]

@locka99 I think (hope) that I addressed all your feedback. I did it in separate commits so it's easy to check, but I also retrofitted the changes into the original 3 commits that also do the rename.

I personally prefer those 3 updated commits as it also updates limits to options which seems to be a better name moving forward. So if/when you are OK with the changes, I can update this PR with those 3 updated commits and mark the PR as ready. But of course I can also just squash the current commits in this PR and use that single commit. Just let me know what you prefer...

EDIT: Thinking about it a bit more, it's probably better to just squash and merge the current commits as this is now a single change. So when your good with the changes I'll only squash the current commits and mark the PR as ready. After that I'll make a separate PR for the rename.

[locka99]

I'll take the limits => options thing separately. It was just the difference between hundreds of files changing vs 17. I'll approve this part and many thanks. I hadn't thought of server / client out of sync and if there is a fix that reconciles the two then maybe it can even become the default at some point.

[locka99]

You'll have to take it out of draft for me to merge it tho

[svanharmelen]

Cool 😀 I squashed the existing commits on this branch to clean things up and took the PR out of draft... So all yours 👍🏻

I have a separate PR for the limits => options ready, but I'll wait with creating the PR so I can rebase it on this one once its merged. Thanks!

[locka99]

I'll run some more test tomorrow to see if I have issues but in the meantime, cheers for the fix. I can take the rename separately

[svanharmelen]

Thanks for merging this one @locka99! It runs in a few locations where we had issues for a few days now and improved things a lot 👍🏻

And just created the PR for the rename the DecodingLimits struct (#107) to finish things off 😉