Bug 19930 - Close() on MUC channels should probably be asynchronous
Summary: Close() on MUC channels should probably be asynchronous
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: gabble (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL: http://git.collabora.co.uk/?p=user/jo...
Whiteboard: review+ with one change
Keywords: patch
Depends on:
Blocks:
 
Reported: 2009-02-03 06:23 UTC by Will Thompson
Modified: 2011-01-13 08:23 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Will Thompson 2009-02-03 06:23:15 UTC
I experienced some weird behaviour when i parted and rejoined a MUC in quick succession. I think that the problem arose because my unavailable presence hadn't got through to the MUC server by the time I tried to rejoin. But Gabble assumes that it gets there immediately, and closes the channel right away. Then it gets confused by the presences for the new session in the MUC arriving.

I think that Close() on a MUC should just send the presence, then wait to get it back; only then should it finish closing and let the MUC factory unref it.
Comment 1 Jonny Lamb 2011-01-11 03:18:43 UTC
Okay I wrote a branch for this, and even a test!

One problem might be that there is currently no timeout on this. If a server doesn't echo your unavailable presence then the channel will never close. I'm not sure if this is a problem yet though. Thoughts?
Comment 2 Will Thompson 2011-01-12 10:46:33 UTC
I think your test should check that Gabble does *something* sensible if you request a new channel while it's waiting for the server to ack you leaving the room.

Possible sensible things include:

1. Just failing the channel request.
2. Queuing the join request.

> One problem might be that there is currently no timeout on this. If a server
> doesn't echo your unavailable presence then the channel will never close. I'm
> not sure if this is a problem yet though. Thoughts?

I think that, given that Gabble bothers to have a 180-second (!) timeout for joining a room in the first place, it should have the same for leaving it.
Comment 3 Will Thompson 2011-01-12 10:54:23 UTC
Oh, and does this deal correctly with:

1. UI calls Close()
2. UI calls Disconnect()
3. Server echoes our presence to say we've left the room

Definitions of correctly include:

• The channel emits Closed() when Disconnect() is called; and doesn't crash when the presence arrives. I believe this works by accident: when Disconnect() is called, the muc factory tells the channel to close without waiting, and then unrefs the channel, which happens to destroy it, which has the effect of destroying the WockyMuc so WockyMuc::parted will not fire and crash us.
• The channel doesn't emit closed until either the presence is echoed, or Gabble gets bored of waiting for the stream close from the server.
Comment 4 Jonny Lamb 2011-01-13 07:20:17 UTC
(In reply to comment #2)
> I think your test should check that Gabble does *something* sensible if you
> request a new channel while it's waiting for the server to ack you leaving the
> room.

Makes sense. I've changed the test to ensure it fails with NotAvailable, and also that it works after the channel has finally closed.

> I think that, given that Gabble bothers to have a 180-second (!) timeout for
> joining a room in the first place, it should have the same for leaving it.

Okay, added. I made it 180s again for consistency. I'm not sure I'd be so popular if I made a test for that.

(In reply to comment #3)
> Oh, and does this deal correctly with:
[snip]
> • The channel emits Closed() when Disconnect() is called; and doesn't crash
> when the presence arrives. I believe this works by accident: when Disconnect()
> is called, the muc factory tells the channel to close without waiting, and then
> unrefs the channel, which happens to destroy it, which has the effect of
> destroying the WockyMuc so WockyMuc::parted will not fire and crash us.

Yeah that's what happened. I added a test for this case as you saw IRL.
Comment 5 Will Thompson 2011-01-13 08:02:47 UTC
   const char *jid = wocky_muc_jid (wmuc);
 
+  DEBUG ("called");
+

Come on. The JID is right there.
Comment 6 Will Thompson 2011-01-13 08:04:12 UTC
otherwise this looks fine
Comment 7 Jonny Lamb 2011-01-13 08:23:30 UTC
le singe est sur la table


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.