During the LibreOffice hackfest, Michael and Eike asked me how reliable tubes are. Specifically, might we ever drop packets? I said I thought that if a message failed to send for whatever reason (maybe rate-limiting by the server) the tube would die, which is imperfect but at least means that as long as the tube is open it's reliable.
I was wrong, at least for in-band bytestreams.
In bytestream-ibb.c's send_data() function, an IQ is constructed with the data to send, and sent (irrelevant bookkeeping elided):
iq = wocky_stanza_build (WOCKY_STANZA_TYPE_IQ, WOCKY_STANZA_SUB_TYPE_SET,
'@', "sid", priv->stream_id,
'@', "seq", seq,
ret = _gabble_connection_send_with_reply (priv->conn, iq, iq_acked_cb,
G_OBJECT (self), NULL, &error);
gabble_bytestream_iface_close (GABBLE_BYTESTREAM_IFACE (self), NULL);
_gabble_connection_send_with_reply() succeeds unless you're literally not connected any more, so the error-checking here is perhaps a little overly keen. But iq_acked_cb doesn't look at the reply to see if it's got type='result' or type='error'. If the IQ is sent successfully but we get an error reply back, we blindly carry on sending subsequent chunks of data. Badness.
(If sending the IQ fails (that is, if we disconnect before we get a reply), the callback is never called. It looks like that's okay in this case.)
I think bytestream-socks5 is okay. bytestream-muc.c uses <message/> stanzas, doesn't put an id='' on them, and doesn't check for errors or for the echo as far as I can tell, so may have similar issues if the MUC rejects some of our messages (rate-limiting doesn't seem unlikely…).
This branch <http://cgit.collabora.com/git/user/wjt/telepathy-gabble/log/?h=47999-handle-ibb-errors> makes the bytestream close up if a stanza gets dropped.
There's no test for this in the branch, because I know Jonny has been gutting the tube tests of references to the old API and I don't want to conflict. I tested this by making the test always send back errors for IBB IQs:
@@ -664,8 +664,7 @@ class BytestreamIBB(Bytestream):
assert ibb_data['sid'] == self.stream_id
# ack the IQ
- result = make_result_iq(self.stream, ibb_event.stanza)
+ send_error_reply(self.stream, ibb_event.stanza)
if len(binary) >= size or size == 0:
received = True
Before this branch, tubes/accept-private-stream-tube.py (which tells Gabble to send some data down the bytestream, waits for it to arrive, sends back a reply over XMPP and sucks the reply out of Gabble) passes even with this change. With this branch, this change breaks this and other tests, as you would expect.
This branch doesn't address MUCs.
I could get behind this. r+
Retitling for MUCs maybe having the same issue (which I still haven't really checked.)