Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Buffer message tags and the original timestamps" #1144

Closed
Robby- opened this issue Oct 6, 2015 · 22 comments
Closed

Revert "Buffer message tags and the original timestamps" #1144

Robby- opened this issue Oct 6, 2015 · 22 comments

Comments

@Robby-
Copy link

Robby- commented Oct 6, 2015

So, I've been running a recent git checkout for a little while now, and there has been some confusion and trouble for users, probably caused by commit c17c8c0.

@jpnurmi: While in theory this looks like a good idea to do, this is not practical, in fact it causes confusion and messes up the time stamps / continuity for clients that request the server-time cap and whose machines have a clock which is off (example provided below).

Don't get me wrong, the buffering of messages with time tags is perfect for buffer playback when you connect to ZNC (like it was before this commit was made), but after the buffer has played back, this time tagging should stop again so clients/users don't get messed up times due to clocks being off compared to the time on the server which ZNC runs on, when they are actively chatting.

An example:

[15:05:33] <timetester> hi
[15:04:36] <timewizard> hey, timetester
[15:05:49] <timetester> how are you today?
[15:04:53] <timewizard> i'm fine, and you?
[15:06:04] <timetester> just got home from...

timetester is a remote user, and timewizard is the user connected to ZNC. This issue also happens for other events, such as quits, kicks, etc.

You could argue that users should fix their clocks, and I agree, but in practice you can't really require this from every ZNC user, and even if they do set up synchronization this issue can still happen due to for example slow intervals between synchronizations (Windows for example does this only once a week, which is plenty of time for the clock to go off again by a few seconds).

So, I would like to request to please revert this commit or make it optional (with the old behavior as the default) if you really want to keep this in.

@Zarthus
Copy link
Contributor

Zarthus commented Oct 7, 2015

I've been noticing the same behaviour with desynced timestamps as well (often just a second or two), I think.. And it happens quite often.

If only mIRC is the suffering client, I think the default option should be on, not off, though.

@Mikaela
Copy link
Contributor

Mikaela commented Oct 7, 2015

ZNC follows the server-time specification and won't be breaking it for users who ignore the documentation where FAQ has My client supports server-time and all timestamps are wrong. Even if the users had read the documentation, they missed bug reporting guidelines entirely.

@Robby-
Copy link
Author

Robby- commented Oct 7, 2015

@Mikaela: As I said earlier, I agree users should take steps to ensure a correct system clock, but I think I've given a strong reason to still revert this to the old behavior nonetheless, namely that clocks still can go off by a few seconds (or even more) in between the intervals, which will again cause issues until the next interval. Or do you honestly expect Windows users to synchronize their clocks every time they intend to connect to ZNC? That will quickly become very annoying. Also expecting every user to do this is not realistic (even with clear instructions). Let alone having them fiddle around in Windows system settings in an attempt to speed up the weekly synchronization interval.

By the way, here is a nice quote from the server-time spec:
Servers MAY include the timestamp in messages when they see fit

The keywords here are MAY and when they see fit. This suggests server and bouncer developers can include time stamps as they themselves see fit. Thus it will not be breaking the spec at all by reverting this commit or turning it into a setting that can be enabled or disabled per user.

Also, here's the info I guess I neglected to include as I thought I already provided enough relevant info in my opening post:
ZNC version: 1.7.x-git-441-d66cb36
Users clients are mIRC version 7.43

@Mikaela
Copy link
Contributor

Mikaela commented Oct 7, 2015

@Mikaela As I said earlier, I agree users should take steps to ensure a correct system clock, but I think I've given a strong reason to still revert this to the old behavior nonetheless, namely that clocks still can go off by a few seconds (or even more) in between the intervals, which will again cause issues until the next interval. Or do you honestly expect Windows users to synchronize their clocks every time they intend to connect to ZNC? That will quickly become very annoying. Also expecting every user to do this is not realistic (even with clear instructions). Let alone having them fiddle around in Windows system settings in an attempt to speed up the weekly synchronization interval.

You are the only person suffering from that issue as all other people asking this question and getting that FAQ entry as reply have been happy with it and not have any issues.

If you have issues with Windows's own NTPd, you are free to replace it with something like the NTPd that is also used on Linux. https://www.meinbergglobal.com/english/sw/ntp.htm

@Robby-
Copy link
Author

Robby- commented Oct 7, 2015

You are making assumptions. I am not the only person, I'm making this request on behalf of other people.

@Mikaela
Copy link
Contributor

Mikaela commented Oct 7, 2015

You are making assumptions. I am not the only person, I'm making this request on behalf of other people.

You are the only person who hasn't been happy with the solution to sync your clock unlike all the people who asked this earlier. There is a reason why it's Frequently Asked Question.

At IRC I said that this also causes breakage to ZNC.

  1. I send something at 12:00:00.
  2. Connection between me and ZNC is lagging and ZNC sends me something received 11:59:58.
  3. I receive the message at 12:00:03, but as my client supports server-time I see correct timestamp of 11:59:58 and it appears before the line I sent at 11:59:58.

If you remove server-time, this won't work, but currently this works as described and is correct.

@Mikaela
Copy link
Contributor

Mikaela commented Oct 7, 2015

I also said at IRC that I have one PC from 2006. It's running systemd-timesyncd with the instructions from previously mentioned FAQ entry (sudo timedatectl set-ntp true) and has never been in wrong time.

@Mikaela
Copy link
Contributor

Mikaela commented Oct 7, 2015

You are the only person who hasn't been happy with the solution to sync your clock unlike all the people who asked this earlier. There is a reason why it's Frequently Asked Question.

At IRC it was pointed out that this commit is not in any stable release, so I am sorry about comments such as this.

@Zarthus
Copy link
Contributor

Zarthus commented Oct 7, 2015

The fact is that it looks odd and will bug anyone who doesn't like
inconsistent timestamps. An option made available to revert the behaviour
would not hurt anyone.

It's really hard for me to care if my clock is off by 2 seconds at maximum.
What is it ultimately going to affect aside from... Apparently znc?

Asking users to install software because the native program doesn't work as
well probably won't be acknowledged and will only prevent an upgrade
because of the questions for support providers will get.

Seeing as znc has worked fine without for so many years, I see nothing
wrong with giving the option.
On 7 Oct 2015 7:05 am, "Mikaela Suomalainen" notifications@github.com
wrote:

You are the only person who hasn't been happy with the solution to sync
your clock unlike all the people who asked this earlier. There is a reason
why it's Frequently Asked Question.

At IRC it was pointed out that this commit is not in any stable release,
so I am sorry about comments such as this.


Reply to this email directly or view it on GitHub
#1144 (comment).

@Mikaela
Copy link
Contributor

Mikaela commented Oct 7, 2015

Asking users to install software because the native program doesn't work as
well probably won't be acknowledged and will only prevent an upgrade
because of the questions for support providers will get.

Please read the whole thread. At IRC it was pointed out that this commit is not in any stable release, so I am sorry about comments such as this.

I thought this was in 1.6.0 or 1.6.1 and that @Robby- somehow suffers from issue which everyone else on #znc has solved by time syncing and I thought that they are unhappy with the NTPd which Windows provides and suggested trying (port of?) the official NTPd which is used on *nix side.

@Zarthus
Copy link
Contributor

Zarthus commented Oct 7, 2015

their original post also implied that they were running on the latest git, strengthened by the commit referenced.

That paragraph referred to the future (i.e. 1.8 release), not 1.6

@Mikaela
Copy link
Contributor

Mikaela commented Oct 7, 2015

I didn't think about clicking the commit and reading what tags include or not it. Specific version information that is requested in CONTRIBUTING.md again was only given in the third commit.

@Mikaela
Copy link
Contributor

Mikaela commented Nov 1, 2015

It appears that I never remembered to paste the #ircv3 discussion on this issue from 2015-10-08.log:

[18:21:32] <Mikaela> If we imagine future where all servers and clients support server-time and all servers include timestamp in all messages and the client runs on system where clock is on wrong time, is server-time inherently broken or is it user issue for not syncing the clock?
[18:21:32] <Mikaela> This question comes from https://github.com/znc/znc/issues/1144 where TL:DR ZNC git attaches timestamp to all messages and user with clock in wrong time says that that behaviour is wrong.
[18:24:41] <Mikaela> by the way, could people who have something to do with IRCv3 be voiced as separation from random idlers etc.?
[18:25:01] <clokep> FWIW I've found it's not uncommon for computers to be pretty off in time.
[18:25:04] <clokep> E.g. 10s of seconds. :-\
[18:25:16] <evilmquin> my initial feeling is it shouldn't be the rest of the internet's problem if you have a broken clock
[18:25:43] <evilmquin> particularly when the computer in question is connected to the internet and plenty of accurate time sources
[18:26:09] <Mikaela> I am used to Linuxes running NTP and using working timeservers and the only place where clock has been on wrong time was Windowses in domain at school. However time.windows.com is inaccurate and Window systems use that by default (when not in domain in which case they use ADDCs)
[18:26:38] <clokep> How are times sent in IRCv3 sent/proposed to be sent? I'm not sure I've come across that spec.
[18:26:48] <evilmquin> you could, I suppose, have the server send the current time sometime around connect and then if they client wants to offset timestamp display based on that, it can
[18:26:53] <ilbelkyr> hm, echo-message combined with server-time would allow clients to at least be able to give sensible timestamps
[18:27:34] <ilbelkyr> (even though I agree "fix your clock" is ultimately highly desirable either way)
[18:28:05] <Mikaela> clokep: http://ircv3.net/specs/extensions/server-time-3.2.html
[18:29:24] <clokep> Mikaela: Neat, I'm glad it's enforced as UTC. I agree with ilbelkyr that it + echo message should solve the issue of time mismatches.
[18:30:01] <Mikaela> Is that documented somewhere I could link and use as confirmation for it not being issue of ZNC?
[18:35:20] <pavonia> Why would you want to use client times instead of the time when a messages reached the server? The client could use arbitrary timestamps for messages
[18:36:06] <Mikaela> pavonia: note that both server and client must be on correct time for times to be shown correctly with server-time
[18:37:17] <evilmquin> does ZNC timestamp all messages when server-time is enabled, or just buffer content?
[18:37:58] <Mikaela> evilmquin: before git only buffer playback, in git all messages
[18:38:27] <pavonia> Mikaela: So when the times mismatch the server won't use the user time?
[18:38:59] <Mikaela> I don't know how to explain it understandably
[18:39:37] <evilmquin> pavonia: where you don't have a server timestamp (either the server didn't send one, or for your own messages in the absense of echo-message), the client will need to use its own clock
[18:40:48] <evilmquin> resulting in the complaint Mikaela linked to, where you have a mix of timestamped and untimestamped messages it looks werid because the clocks aren't in sync
[18:41:22] *** Joins: robotoad (~robotoad@174.137.69.7)
[18:41:24] <pavonia> Ah, so the server never uses a client's timestamp but vice versa?
[18:42:02] <evilmquin> yeah, these are only sent server->client
[18:42:11] <pavonia> I see
[18:43:07] <clokep> Make that spec require echo-message? Or add a note to that spec that it's an issue unless echo-message is used?

@Mikaela
Copy link
Contributor

Mikaela commented Nov 22, 2015

make it optional (with the old behavior as the default) if you really want to keep this in.

If this happens, it must be user level setting so admin cannot break ZNC for users who have their clocks on correct time or use echo-message capable clients.

I also disagree with having the old behaviour as default (but I also disagree with the whole issue being issue).

@Zarthus
Copy link
Contributor

Zarthus commented Nov 22, 2015

Why do you disagree on the default? Should the "always functioning" behaviour not take precedence over the "often works and causes annoyance in the case it doesn't" behaviour?

Most users will not exactly know what is going on and end up confused. To me, this comes across as a bug very often and would lead to many false bug reports.

@Mikaela
Copy link
Contributor

Mikaela commented Nov 22, 2015

Because that is workaround for broken behaviour causing incorrect behaviour on proper clients.

I won't be commenting this issue anymore until the IRCv3 working group has decided on ircv3/ircv3-specifications#182 and preferably written to the server-time specification what is the right thing to do.

@attilamolnar
Copy link

It's up to the client to interpret the timestamps, ZNC is doing nothing wrong here. The client can choose to unwisely display timestamps that refer to the future as they are or, more sensibly, warn the user or ignore the timestamps in this case altogether.

@Zarthus
Copy link
Contributor

Zarthus commented Nov 22, 2015

It's not timestamps from the future. It's often timestamps from the past, and in general just leading to undesirable behaviour. The previous behaviour worked fine for me - znc forcing this new thing onto me without a setting to turn it off is bad.

If the previous behaviour has worked well since the software was made, I see no reason to force a new change of behaviour onto us that seems to be less ideal and leads to much confusion. It's the same way how encoding defaults to "legacy, not recommended" and assumes the user to set it correctly.

Occurance today: I could not verify if an user was using autorejoin-on-kick because the timestamp of the rejoin was 1 second before the kick.

@attilamolnar
Copy link

Why do you disagree on the default? Should the "always functioning" behaviour not take precedence over the "often works and causes annoyance in the case it doesn't" behaviour?

What you are saying is already the case, but not exactly as you imagine. The default "always functioning" behavior is that server-time (the feature causing these timestamps to be sent) is not enabled for any client. A client may explicitly opt-in to this truly optional feature which causes the timestamps to be sent. How this feature works if a client requests it is documented, ZNCs both old and new behavior conforms to this specification.

If the previous behaviour has worked well since the software was made, I see no reason to force a new change of behaviour onto us that seems to be less ideal and leads to much confusion. It's the same way how encoding defaults to "legacy, not recommended" and assumes the user to set it correctly.

Encoding defaults to legacy because it's non-trivial or impossible to figure out the encoding when it's not signaled explicitly. This is not similar to server-time, because with server-time there is nothing to figure out. It works as specified.

I think the request to add such a switch to ZNC is similar to asking email servers to not send the timestamp of an email to the client because it could look odd when displayed to the user if the user's clock is wrong. But the way it works is that the email client has the timestamp anyway and figures out what's best to do with it. It's true that for email a few seconds often don't make a difference but it's the same idea.

I've picked email as an example, you can check out whether you see the correct timestamp in WhatsApp, Skype, Facebook Chat etc. if your clock is wrong.

Occurance today: I could not verify if an user was using autorejoin-on-kick because the timestamp of the rejoin was 1 second before the kick.

If the server sends timestamps in all messages then this suggests broken behavior from the client as a KICK only happens when the server sends the KICK, not when the client sends the KICK to the server. Therefore, if the server sent timestamps in both cases (KICK+JOIN) the client either ignored one of them or displayed the kick prematurely, before it actually happened.

@attilamolnar
Copy link

I do acknowledge the existence of the problem, but I believe this issue is knocking on the wrong door.

@Robby-
Copy link
Author

Robby- commented Nov 23, 2015

At the very least, having some notification from ZNC to the client when all buffers have played back would be nice to have, so we could have a client-side script listening for this special notification and then turn off the server-time cap again. This way we can still have the nice integration into our client that this cap offers (because during buffer playback I do not mind that the clock is off a little bit), and not have the annoyance of inconsistent time stamps afterwards.

@DarthGandalf
Copy link
Member

@Robby- ZNC sends playback in a batch, if client supports batches.
Your script can certainly watch for end of the batch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants