FEAT(server): Add rememberduration option #4147

deluxghost · 2020-05-08T07:11:37Z

This option allows to set a threshold on how long a user's channel
should be remembered. This is useful for scenarios where users usually
don't want their channel to be remembered by the server unless they had
a disconnect (aka have ot re-connect after a short period of time).

Implements #4143

deluxghost · 2020-05-08T10:06:12Z

@Krzmbrzl updated

Krzmbrzl

Could you also do 2 more things, please?

Remove the unrelated whitespace changes ( I guess you have used some sort of auto-formatter?)
Prefix your commit message with src/murmur:

src/murmur/Messages.cpp

Krzmbrzl

I assume you have tested this to be working (using different values for rememberduration including disabling and not setting it), right?

deluxghost · 2020-05-10T16:13:08Z

I assume you have tested this to be working (using different values for rememberduration including disabling and not setting it), right?

Wait, I think I found a problem

deluxghost · 2020-05-10T16:22:36Z

OK, the problem is the last_active in the user table is updated while a user enters a channel, instead of leaving it. The behavior blocks this PR currently.

Should I update that value upon user leaving (by any means)? Any tips?

Krzmbrzl · 2020-05-10T17:14:57Z

The behavior blocks this PR currently.

How so? Because the time difference is now between rejoining and entering a channel vs. leaving it?

Hm... A quick check revealed that last_active is used in several places, so I'm not sure whether it's a good idea to temper with it. Who knows: Maybe for one of these situations it's actually important for this timestamp to represent the last time a channel was joined...

I guess the easiest way would be to change the documentation of that option to state that the time interval is relative to when that channel was joined. If we also change the unit of that option to be hours, I think the difference doesn't matter that much in a lot of cases.
It's a bit of a sloppy workaround though 🤔

deluxghost · 2020-05-10T17:41:28Z

I think that will be a bit strange. If the user has been in the channel for a long time (> duration), he won't auto re-join upon he lost his connection accidentally. but if we change the duration to a large value just for that case, the option will become meaningless. The remember feature is only useful in several minutes after disconnecting, similar to multiplayer games.

I think we still need a way to get the time of user leaving, that makes more sense, i.e. "when you lost connection, you have x minutes to reconnect, we will take you to the last channel."

Is is easy to add a new column to represent user leaving time?

Krzmbrzl · 2020-05-11T12:00:54Z

Is is easy to add a new column to represent user leaving time?

Yeah it should be relatively straight forward. You just have to make sure that any existing table will be extended by that new column as well. That's part of the whole setup code that is at the top of ServerDB.cpp. I haven't done much with that yet, so I'll have to look into this as well 👀

deluxghost · 2020-05-12T07:52:03Z

Looks like the whole setup code is trying to modify db manually by the internal "version"?

I guess what I need to do is

bumping current db version to 7 (?)
adding the new column if version<7
creating new table with the new column if version==0 (I assume this means no db exists?)

It's difficult to read anything from tons of SQL queries... I still prefer any migration mechanism

Krzmbrzl · 2020-05-13T16:32:02Z

Sounds about right, yes. And while you're on it, it might be a good idea to create a member variable that holds the current db version...

It's difficult to read anything from tons of SQL queries...

Agreed. The code is quite messy

Krzmbrzl · 2020-05-24T18:11:51Z

@deluxghost any (planned) progress here? :)

deluxghost · 2020-05-24T18:17:27Z

@Krzmbrzl uhh sorry for the delay. I was just too excited to learn the new Hammer tools for Half Life Alyx recently. I am going to come back to this PR asap.

Krzmbrzl · 2020-05-24T18:46:09Z

No problem. Just wanted to get a status update to see if this PR might be abandoned :)

Krzmbrzl · 2020-05-29T15:38:17Z

Sounds about right, yes. And while you're on it, it might be a good idea to create a member variable that holds the current db version...

I actually went ahead and did that in #4220

There I also had to dig through the database code, so now I know how stuff works in there. This means I'll be able to better assist you with this project now :D

creating new table with the new column if version==0

I think you should instead create a new column last_seen in the users table that'll get updated every time a user disconnects. That way we have exactly the time we wanted ☝️

deluxghost · 2020-05-29T16:01:06Z

uh yes, that's what i'm going to do

btw, is there any simple db editor with GUI for debugging purposes?

Krzmbrzl · 2020-05-29T17:18:39Z

For SQLite (which is the default backend) there seems to be https://sqlitebrowser.org/

deluxghost · 2020-06-01T16:57:20Z

updated something, but i'm not sure if i did it right.

and it might make sense to update last_seen of every online user while server shutting down?

Krzmbrzl

Furthermore you'll also have to change the database version (from 7 to 8) in ServerDB.h and you'll also make sure that the upgrade process involves copying the new column as well (I'm not sure right now whether all columns are copied automatically or whether that has to be done explicitly)

scripts/murmur.ini

src/murmur/ServerDB.cpp

src/murmur/Server.h

Krzmbrzl · 2020-06-02T05:57:25Z

and it might make sense to update last_seen of every online user while server shutting down?

Absolutely, yes

deluxghost · 2020-06-02T07:15:30Z

and it might make sense to update last_seen of every online user while server shutting down?

Absolutely, yes

for this, where should I look into? i don't understand where the server handles the shutting down process

you'll also make sure that the upgrade process involves copying the new column as well

it uses INSERT INTO to import old data:

mumble/src/murmur/ServerDB.cpp

Lines 603 to 606 in 7cd65f3

    
           if (version < 4) 
        
           	SQLDO("INSERT INTO `%1users` (`server_id`, `user_id`, `name`, `pw`, `lastchannel`, `texture`, `last_active`) SELECT `server_id`, `player_id`, `name`, `pw`, `lastchannel`, `texture`, `last_active` FROM `%1players%2`"); 
        
           else 
        
           	SQLDO("INSERT INTO `%1users` (`server_id`, `user_id`, `name`, `pw`, `lastchannel`, `texture`, `last_active`) SELECT `server_id`, `user_id`, `name`, `pw`, `lastchannel`, `texture`, `last_active` FROM `%1users%2`");

should I write like this?

if (version < 4)
  // sql
else if (version < 8)
  // sql
else
  // sql with last_seen

Krzmbrzl · 2020-06-02T07:27:10Z

for this, where should I look into? i don't understand where the server handles the shutting down process

Actually when thinking about this, it might get complicated as you'd have to make sure that you catch the server before it's actually shutting down in order to access the user-list...

I think we might be better off if we ignore the shutdown and instead clear the last_seen field when the user connects (in order to not use an outdated date if the server is shut down).
In this context it might even be a good idea to use a different name for the column. Something like last_disconnect maybe? 🤔

should I write like this?

Yes

deluxghost · 2020-06-07T14:17:55Z

I think we might be better off if we ignore the shutdown and instead clear the last_seen field when the user connects

Looks like this method won't prevent using the old value, because we should use the value first, before clear it (or the valid values will be ignored too).

I think we should clear last_disconnect of every user when starting the server? It might take time, but I don't have a better idea.

If we should do this, could you tell me which part of the code should I look at, please?

deluxghost · 2020-06-07T14:33:35Z

oh, wait, if we don't clear last_disconnect manually, the only problem is the server might forget users' last channel earlier than expected, which isn't a big problem.

so just ignore these things is also an option.

Krzmbrzl · 2020-06-07T17:11:00Z

Looks like this method won't prevent using the old value, because we should use the value first, before clear it (or the valid values will be ignored too).

Indeed 😄

I think we should clear last_disconnect of every user when starting the server? It might take time, but I don't have a better idea.

That sounds like a good idea to me 🤔

oh, wait, if we don't clear last_disconnect manually, the only problem is the server might forget users' last channel earlier than expected, which isn't a big problem.

True, but if we can fix that, I think it'd still be better to do so :)

If we should do this, could you tell me which part of the code should I look at, please?

I think it could be as simple as adding a new slot to the ServerDB class and connect that to Meta::started(Server *server) signal (you can do the connection in main.cpp as soon as the Meta object and the ServerDB object exist).

This should be after this line:

mumble/src/murmur/main.cpp

Line 455 in 6dd5db7

meta = new Meta();

Krzmbrzl · 2020-06-20T09:45:18Z

@deluxghost what's the status here?

deluxghost · 2020-06-20T14:15:13Z

updated

I spent more time on finding out how the hell the moc works 😦

Krzmbrzl · 2020-06-20T14:34:10Z

Okay nice 👍
I'll review it in the upcoming days :)

src/murmur/ServerDB.cpp

This option allows to set a threshold on how long a user's channel should be remembered. This is useful for scenarios where users usually don't want their channel to be remembered by the server unless they had a disconnect (aka have ot re-connect after a short period of time). Implements #4143

Krzmbrzl · 2020-07-03T17:30:55Z

@deluxghost I took the liberty of adjusting your commit message to fit our new commit guidelines ☝️

Krzmbrzl · 2020-07-08T17:21:43Z

Thank you very much for your contribution! :)

The previous implementation of rememberchannelduration (mumble-voip#4147) acknowledged the unreliability of the last_disconnect database field. It used a workaround to set the last_disconnect field to NULL for every user on every start of the mumble server. However, with a default behavior for NULL, this created the unintended side-effect that rememberchannel was erroneously used for every user on their first connection after a server restart. This new implementation reverts the workaround of mumble-voip#4147 and uses the last_active database field to check if last_disconnect is reliable. If last_disconnect is unreliable, it will now use the server uptime and compare it to rememberchanneduration. We expect last_disconnect to be unreliable when the server was shut down while clients are still connected. Fixes mumble-voip#5639

…logic The previous implementation of rememberchannelduration (#4147) acknowledged the unreliability of the last_disconnect database field. It used a workaround to set the last_disconnect field to NULL for every user on every start of the mumble server. However, with a default behavior for NULL, this created the unintended side-effect that rememberchannel was erroneously used for every user on their first connection after a server restart. This new implementation reverts the workaround of #4147 and uses the last_active database field to check if last_disconnect is reliable. If last_disconnect is unreliable, it will now use the server uptime and compare it to rememberchanneduration. We expect last_disconnect to be unreliable when the server was shut down while clients are still connected. Fixes #5639

The previous implementation of rememberchannelduration (mumble-voip#4147) acknowledged the unreliability of the last_disconnect database field. It used a workaround to set the last_disconnect field to NULL for every user on every start of the mumble server. However, with a default behavior for NULL, this created the unintended side-effect that rememberchannel was erroneously used for every user on their first connection after a server restart. This new implementation reverts the workaround of mumble-voip#4147 and uses the last_active database field to check if last_disconnect is reliable. If last_disconnect is unreliable, it will now use the server uptime and compare it to rememberchanneduration. We expect last_disconnect to be unreliable when the server was shut down while clients are still connected. Fixes mumble-voip#5639 (cherry picked from commit ccbf407)

deluxghost changed the title ~~feat: remember channel duration~~ WIP: feat: remember channel duration May 8, 2020

deluxghost mentioned this pull request May 8, 2020

Configurable timeout for rememberchannel #4143

Closed

deluxghost changed the title ~~WIP: feat: remember channel duration~~ feat: remember channel duration May 10, 2020

Krzmbrzl requested changes May 10, 2020

View reviewed changes

src/murmur/Messages.cpp Show resolved Hide resolved

Krzmbrzl approved these changes May 10, 2020

View reviewed changes

Krzmbrzl added the server label May 10, 2020

Krzmbrzl added this to the 1.4.0 milestone May 10, 2020

deluxghost changed the title ~~feat: remember channel duration~~ WIP: feat: remember channel duration May 13, 2020

Krzmbrzl requested changes Jun 2, 2020

View reviewed changes

scripts/murmur.ini Outdated Show resolved Hide resolved

src/murmur/ServerDB.cpp Outdated Show resolved Hide resolved

src/murmur/ServerDB.cpp Show resolved Hide resolved

src/murmur/ServerDB.cpp Outdated Show resolved Hide resolved

src/murmur/Server.h Outdated Show resolved Hide resolved

Krzmbrzl reviewed Jun 22, 2020

View reviewed changes

src/murmur/ServerDB.cpp Outdated Show resolved Hide resolved

deluxghost changed the title ~~WIP: feat: remember channel duration~~ feat: remember channel duration Jul 3, 2020

Krzmbrzl approved these changes Jul 3, 2020

View reviewed changes

Krzmbrzl changed the title ~~feat: remember channel duration~~ FEAT(server): Add rememberduration option Jul 8, 2020

Krzmbrzl merged commit 9d24422 into mumble-voip:master Jul 8, 2020

Hartmnt mentioned this pull request Apr 29, 2022

rememberchannelduration option is ignored after restarting murmur #5639

Closed

FEAT(server): Add rememberduration option #4147

FEAT(server): Add rememberduration option #4147

deluxghost commented May 8, 2020 •

edited by Krzmbrzl

deluxghost commented May 8, 2020

Krzmbrzl left a comment

Krzmbrzl left a comment

deluxghost commented May 10, 2020

deluxghost commented May 10, 2020

Krzmbrzl commented May 10, 2020

deluxghost commented May 10, 2020

Krzmbrzl commented May 11, 2020

deluxghost commented May 12, 2020

Krzmbrzl commented May 13, 2020

Krzmbrzl commented May 24, 2020

deluxghost commented May 24, 2020

Krzmbrzl commented May 24, 2020

Krzmbrzl commented May 29, 2020

deluxghost commented May 29, 2020

Krzmbrzl commented May 29, 2020

deluxghost commented Jun 1, 2020

Krzmbrzl left a comment

Krzmbrzl commented Jun 2, 2020

deluxghost commented Jun 2, 2020

Krzmbrzl commented Jun 2, 2020 •

edited

deluxghost commented Jun 7, 2020

deluxghost commented Jun 7, 2020

Krzmbrzl commented Jun 7, 2020 •

edited

Krzmbrzl commented Jun 20, 2020

deluxghost commented Jun 20, 2020

Krzmbrzl commented Jun 20, 2020

Krzmbrzl commented Jul 3, 2020

Krzmbrzl commented Jul 8, 2020

FEAT(server): Add rememberduration option #4147

FEAT(server): Add rememberduration option #4147

Conversation

deluxghost commented May 8, 2020 • edited by Krzmbrzl

deluxghost commented May 8, 2020

Krzmbrzl left a comment

Choose a reason for hiding this comment

Krzmbrzl left a comment

Choose a reason for hiding this comment

deluxghost commented May 10, 2020

deluxghost commented May 10, 2020

Krzmbrzl commented May 10, 2020

deluxghost commented May 10, 2020

Krzmbrzl commented May 11, 2020

deluxghost commented May 12, 2020

Krzmbrzl commented May 13, 2020

Krzmbrzl commented May 24, 2020

deluxghost commented May 24, 2020

Krzmbrzl commented May 24, 2020

Krzmbrzl commented May 29, 2020

deluxghost commented May 29, 2020

Krzmbrzl commented May 29, 2020

deluxghost commented Jun 1, 2020

Krzmbrzl left a comment

Choose a reason for hiding this comment

Krzmbrzl commented Jun 2, 2020

deluxghost commented Jun 2, 2020

Krzmbrzl commented Jun 2, 2020 • edited

deluxghost commented Jun 7, 2020

deluxghost commented Jun 7, 2020

Krzmbrzl commented Jun 7, 2020 • edited

Krzmbrzl commented Jun 20, 2020

deluxghost commented Jun 20, 2020

Krzmbrzl commented Jun 20, 2020

Krzmbrzl commented Jul 3, 2020

Krzmbrzl commented Jul 8, 2020

deluxghost commented May 8, 2020 •

edited by Krzmbrzl

Krzmbrzl commented Jun 2, 2020 •

edited

Krzmbrzl commented Jun 7, 2020 •

edited