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

Do not part from channels in destroy handler #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

daniellandau
Copy link
Contributor

In case the client suddenly disappears (e.g. crashes) the user was
previously parted from the channels. This is terrible behaviour when the
user is using a bouncer, as they expect to stay in their channels. This
handler doesn't seem to be called on clean exit of clients, only when
they disappear suddenly.

This bug: https://bugs.freedesktop.org/show_bug.cgi?id=98685

In case the client suddenly disappears (e.g. crashes) the user was
previously parted from the channels. This is terrible behaviour when the
user is using a bouncer, as they expect to stay in their channels. This
handler doesn't seem to be called on clean exit of clients, only when
they disappear suddenly.
@gkiagia
Copy link
Member

gkiagia commented Feb 5, 2017

So, the motive for this patch makes total sense, but the actual change is more tricky than it seems.

FYI, the _destroy() method is documented here: https://telepathy.freedesktop.org/spec/Channel_Interface_Destroyable.html#Method:Destroy

As the spec says, this method is meant to be called when the channel should close and all pending messages should be discarded. This is being called by mission-control when the handler crashes, in order to avoid respawning the handler and potentially doing an infinite loop of crashing and respawning... By removing the "part" command, this method practically becomes a no-op; destroy() no longer does anything, so it is incorrect. It should, at least, discard any pending messages.

@gkiagia gkiagia self-requested a review February 5, 2017 11:40
@daniellandau
Copy link
Contributor Author

In essence part_from_channel does (in pseude code, the real thing is in idle-muc-channel.c:1284)

	send_command(idleMUCChannel, "PART [channel name]");

so it's not not discarding any pending messages now either.

Even an infinite of crashing and respawning is better than the current behavior, which in essence is irrevocable data loss, as the user gets parted from channels in their bouncer, possibly without them noticing and they might lose a lot of messages.

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.

2 participants