[Bug 33460] IM and MUC channels should use wocky_porter_send_async(), and emit failing delivery reports

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=33460

--- Comment #2 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2011-04-29 04:56:24 PDT ---
> Fixes : fd.o#33460 - IM and MUC channels should use wocky_porter_send_async(),
> and emit failing delivery reports
> Please enter the commit message for your changes. Lines starting

That's not a particularly good commit message (and you've accidentally included
part of the template). See, for instance,
<http://spheredev.org/wiki/Git_for_the_lazy#Writing_good_commit_messages>.
Something like this would be better:

> fd.o#33460: use wocky_porter_send_async() for IM and MUC channels
>
> This lets us send delivery reports for some failure modes, in particular
> if we queue a message for sending but then get disconnected before it
> reaches the server.

... except that it would still be a lie, because you've actually only patched
IM channels so far, not MUC channels!

You're not meant to send failed delivery reports for messages that you haven't
signalled as being set. There are two valid things you can do, depending which
behaviour we want on D-Bus.

(Approach 1) Don't return from SendMessage or emit MessageSent until the
message has traversed Wocky's queue and reached the (TCP socket to the) server.
If the socket accepts the stanza, return from SendMessage and emit MessageSent;
if we get disconnected before the socket accepts the stanza (i.e. Wocky gives
us an async error), make SendMessage fail and don't emit MessageSent (i.e. the
message was never sent). To implement this, in
_gabble_im_channel_message_sent_cb you would call tp_message_mixin_sent() with
error = NULL on success, or call tp_message_mixin_sent() with error != NULL on
failure, and not bother with delivery reports at all.

(Approach 2) Return from SendMessage and emit MessageSent immediately. If the
socket accepts the stanza, either do nothing, or emit an Accepted
delivery report; if we get disconnected before the socket accepts the stanza,
emit a Permanently_Failed delivery report. To implement this, you would call
tp_message_mixin_sent() with error = NULL at the same time as calling
wocky_porter_send_async(), and then in _gabble_im_channel_message_sent_cb you
would emit a delivery report.

What you've implemented seems to be an incorrect mixture of those approaches,
as far as I can see.

I think Approach 1 is the correct one for this situation: conceptually, the
message was never sent, because we got disconnected before we could send it, so
the server never even saw it.

> +gabble_message_util_prepare_send_message (TpMessage *message,

This looks suspiciously as though you've duplicated or reimplemented most of
gabble_message_util_send_message (so I'm not going to review its implementation
in detail immediately). Couldn't you make gabble_message_util_send_message call
gabble_message_util_prepare_send_message (i.e. move the code), instead?

The sequence of commits I'd expect for this bug would go something like this:

* factor out gabble_message_util_prepare_send_message() from g_m_u_s_m()
* make IM channels use g_m_u_p_s_m() instead of g_m_u_s_m()
* make MUC channel use g_m_u_p_s_m() instead of g_m_u_s_m()
* delete g_m_u_s_m()

The name prepare_send_message() doesn't really describe what this function
does: what it actually does is to serialize a TpMessage (D-Bus) into a
LmMessage (XML).

Also, LmMessage is just a backwards-compat typedef for WockyStanza (since
Gabble 0.9), so call it WockyStanza in all new APIs, and use Wocky functions
instead of the equivalent LM functions in new code.

I'd tend to call it gabble_message_util_build_stanza() or something, and make
it return the WockyStanza or NULL on error, like this:

WockyStanza *gabble_message_util_build_stanza (TpMessage *message,
    ...[all the other "in" arguments]...,
    gchar **token,
    GError **error);

> + flags = TP_MESSAGE_SENDING_FLAG_REPORT_DELIVERY;

This doesn't seem to do anything, apart from confusing readers, because flags
isn't passed to anything. Don't change the value of flags here: it's meant to
be the flags you were given from D-Bus, indicating how much the sender cares
about delivery reporting.

(In some protocols, we'd send the message in a "cheap" way if Report_Delivery
isn't present in @flags, or in an "expensive" way that has better tracking if
Report_Delivery is present. In the subset of XMPP that we currently implement,
there's no difference.)

> + gabble_message_util_prepare_send_message (message,
> + GABBLE_CONNECTION (tp_base_channel_get_connection (base)),
> + 0, state, priv->peer_jid, priv->send_nick, &msg, &id, &error);
> +
> + gabble_conn =
> + GABBLE_CONNECTION (tp_base_channel_get_connection (base));

Move gabble_conn a bit higher up and you can use it as the second parameter to
g_m_u_p_s_m(), instead of duplicating the checked cast. (GABBLE_CONNECTION() is
a checked cast similar to qobject_cast<> or dynamic_cast<>, not a cheap
compile-time thing like static_cast<>.)

> + /* FIXME: What to do with the error ?, How should I handle it? */

tp_message_mixin_sent (object, message, flags, NULL, error), which means "this
message was not sent, because @error", and causes the SendMessage D-Bus method
to raise a D-Bus error based on @error. You must call tp_message_mixin_sent()
exactly once per call to _gabble_im_channel_send_message(), either immediately
like this, or in an async callback.

> + TpMessage *message = chan->priv->message;

You can't store the message in the channel like this: sending more than one
message at the same time is (or should be!) allowed. The user_data probably
needs to be a "context" struct containing references to both the message and
the channel.

> + TpHandle sender = tp_base_connection_get_self_handle (base_conn);
> + TpDeliveryStatus delivery_status = TP_DELIVERY_STATUS_PERMANENTLY_FAILED;
> + TpChannelTextSendError send_error = TP_CHANNEL_TEXT_SEND_ERROR_OFFLINE;

I'd move all three of these into the "else" block: they're not relevant if
wocky_porter_send_finish() succeeds.

> + TpMessage *message = chan->priv->message;
...
> + g_object_unref (chan->priv->message);

Both tp_cm_message_take_message() and tp_message_mixin_sent() consume one ref
to the message, so this unref is wrong either way.

> +void gabble_message_util_add_chat_state (LmMessage *msg,
> + TpChannelChatState state);

Minor thing: I don't object to the rename if you want to do that, but I don't
see why this needs to become extern - as far as I can see, only things in the
same translation unit (.c file) ever call it, so it can stay static?

>  /**
>   * _gabble_im_channel_receive
> - *
> + *:
>   */

What's this supposed to achieve? If you're going to change this content-free
comment at all, please either delete the whole comment because it doesn't
contain any information, or fill in some useful documentation. (For the
purposes of this bug, just leave it untouched, though.)

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.