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

WIP: Make conferences persistent across restarts and reconnects. #826

Closed
wants to merge 2 commits into from

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Feb 28, 2018

This is not new groupchats. This is just upgrade to old groupchats with
some advantages:

  • Groupchats are now saved into tox_save.
  • Clients can get groupchat unique id to save message log.
  • Auto restore groupchats after restart even your friend uses
    non-upgraded version.

Submission will happen through #295. Review this PR only.


This change is Reviewable

@iphydf iphydf added this to the v0.2.x milestone Feb 28, 2018
@JFreegman
Copy link
Member

toxcore/group.c, line 1181 at r1 (raw file):

    for (uint16_t i = groupnumber + 1; i < g_c->num_chats; ++i) {
        if (g_c->chats[i].live) {
            return 0;

When kill_groupchats() is called from tox_kill(), this function fails to free the memory associated with some of the groups (via realloc_conferences() below) because of this line. I'm not sure what the live variable indicates but all memory associated with conferences should be freed on shutdown regardless of the state. Commenting this line out fixes this particular memory leak, but causes others elsewhere.


Comments from Reviewable

@sudden6
Copy link

sudden6 commented Mar 3, 2018

Reviewed 12 of 14 files at r1.
Review status: 12 of 14 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


toxcore/group.c, line 2382 at r1 (raw file):

static bool self_peer_gid_collision(Group_c *g)
{
    uint32_t me = ~0;

use UINT32_MAX


toxcore/group.c, line 2586 at r1 (raw file):

        for (j = 0; j < DESIRED_CLOSE_CONNECTIONS; ++j) {
            if (closest(g, j) && g->closest_peers[j] == i) {
                j = DESIRED_CLOSE_CONNECTIONS + 100;

I think this magic + 100 thing is only used to get into the condition below, would be better to replace it with a flag variable


toxcore/group.c, line 2592 at r1 (raw file):

        }

        if (j == DESIRED_CLOSE_CONNECTIONS + 100) {

here


Comments from Reviewable

@iphydf iphydf force-pushed the isotoxin-groupchat branch 2 times, most recently from ada0494 to 262b7f6 Compare March 9, 2018 13:19
@iphydf
Copy link
Member Author

iphydf commented Mar 9, 2018

Review status: 11 of 14 files reviewed at latest revision, 4 unresolved discussions.


toxcore/group.c, line 2382 at r1 (raw file):

Previously, sudden6 wrote…

use UINT32_MAX

Done.


toxcore/group.c, line 2586 at r1 (raw file):

Previously, sudden6 wrote…

I think this magic + 100 thing is only used to get into the condition below, would be better to replace it with a flag variable

Done.


toxcore/group.c, line 2592 at r1 (raw file):

Previously, sudden6 wrote…

here

Done.


Comments from Reviewable

@sudden6
Copy link

sudden6 commented Mar 9, 2018

This were the last things I saw, from my side :lgtm_strong:


Review status: 11 of 14 files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@iphydf iphydf modified the milestones: v0.2.1, v0.2.x Mar 9, 2018
@iphydf iphydf force-pushed the isotoxin-groupchat branch 5 times, most recently from 53b9557 to cdf2e97 Compare March 18, 2018 04:58
@iphydf iphydf force-pushed the isotoxin-groupchat branch 4 times, most recently from 1163e19 to 20ef928 Compare April 6, 2018 12:56
@gongzisun
Copy link

Review a month, how is it going?

@tox-user
Copy link
Member

tox-user commented Apr 27, 2018

I've been testing persistent groups with toxcore 0.2.2 for a week now. The biggest surprise for me was that they work even if I'm the only peer in a group who uses them. Other peers in a group don't need to have toxcore with persistent group chats to allow me to reconnect automatically. The biggest problem that I've noticed is that there is no indication that someone left the group while I was offline. Which means I will often wait to get reconnected, but it might never happen, because the other peers might have left. This could be partially fixed by clients. Instead of creating a persistent group only when we are invited to it, they should be always created on client start. That way it would be visible that we are in a group, even when none of the peers are online or they have left the group.

@envsh
Copy link

envsh commented May 16, 2018

The biggest problem that I've noticed is that there is no indication that someone left the group while I was offline.

for persistent conference, seems need some way to get persistent peer list, and every peers online status.

@tox-user
Copy link
Member

Yes, that would be great to have at some point.

I also have some weird issues with toxync and persistent groups. I don't get reconnected to toxync groups with autoinvite disabled when I come online, but when I then ask toxync to send me an invite to that group, it gets accepted without my permission. It's very strange, because it only happens with toxync groups.

@iphydf iphydf force-pushed the isotoxin-groupchat branch 5 times, most recently from b53e46d to 8351f6c Compare May 20, 2018 16:57
@iphydf iphydf force-pushed the isotoxin-groupchat branch 4 times, most recently from dcc1c44 to c63ad51 Compare July 15, 2018 22:46
@iphydf iphydf changed the title Make conferences persistent across restarts and reconnects. (not WIP, but review-only) WIP: Make conferences persistent across restarts and reconnects. Jul 21, 2018
@iphydf iphydf force-pushed the isotoxin-groupchat branch 5 times, most recently from af4a117 to a9405d7 Compare July 28, 2018 17:17
@iphydf iphydf force-pushed the isotoxin-groupchat branch 3 times, most recently from 38abb1e to 7df5404 Compare August 4, 2018 13:18
@iphydf iphydf force-pushed the isotoxin-groupchat branch 5 times, most recently from bdd92e6 to 7f65bb9 Compare August 12, 2018 16:31
isotoxin and others added 2 commits August 12, 2018 22:30
This is not new groupchats. This is just upgrade to old groupchats with
some advantages:
- Groupchats are now saved into tox_save.
- Clients can get groupchat unique id to save message log.
- Auto restore groupchats after restart even your friend uses
  non-upgraded version.
The crash happened, because the 'free_peer_members' function didn't set
'nick_len = 0'
@iphydf
Copy link
Member Author

iphydf commented Aug 12, 2018

Superseded by #1069.

@iphydf iphydf closed this Aug 12, 2018
@robinlinden robinlinden modified the milestones: v0.2.x, v0.2.6, meta Aug 14, 2018
@iphydf iphydf deleted the isotoxin-groupchat branch January 10, 2022 01:42
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

Successfully merging this pull request may close these issues.

None yet

8 participants