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

Reimplement send_with_reply() using DBusPendingCall #86

Closed
wants to merge 2 commits into from

Conversation

albel727
Copy link
Contributor

Current implementation of send_with_reply() relies on send() + adding the returned MessageReply to msg_handlers() array. There are a few downsides to this approach.

  1. Makes user go through two steps to achieve the intended result.
  2. Relies on filter_message_cb() not getting called until user adds MessageReply to msg_handlers(), otherwise the message will be missed and the handler will live forever. I.e. it lacks atomicity.
  3. Doesn't handle Error replies at all, which means a leaked MessageReply per every failed call.
  4. send_with_reply() is inconvenient to use while looping with iter(), because for some reason msg_handlers() is a property of ConnectionItems, not Connection, so the idiomatic for i in c.iter(1000) means the currently acting ConnectionItems is consumed, and so one can't call its msg_handlers() property from inside the loop, unless the loop is rewritten with manual use of next().

This PR reimplements send_with_reply() in terms of dbus_connection_send_with_reply() and DBusPendingCall, so that none of the above is an issue anymore.

Downsides of this PR: MessageReply is removed altogether, and send_with_reply() has the return type changed. I.e. this is an API-breaking change.

Things to consider: If signature change is OK, maybe we should consider exposing the timeout parameter of dbus_connection_send_with_reply() too.

@albel727
Copy link
Contributor Author

Lol, just noticed the PR #76, which is almost identical. Let me address some of concerns from there.

Finally, I assume that the response from dbus_pending_call_steal_reply() won't be null, since it is only called when notified.

I believe this is a wrong assumption since the callback is also called on timeout, but anyhow, this is likely irrelevant in case the patch is rewritten to not use the dbus_pending_call API.

No, it's actually alright, because libdbus synthesizes a timeout error message in this case, see _dbus_pending_call_set_timeout_error_unlocked().

I actually wanted to test it, but libdbus timeouts simply don't work, because you don't use dbus_connection_set_timeout_functions(), so exposing timeout parameter to send_with_reply() function would be useless at the moment.

Anyway, since you rejected this approach in the past in favor of own filtering, it's probably unlikely that you'll accept this PR. Oh well, maybe it does make more sense to do everything manually, to decouple from libdbus more. Feel free to close this PR.

@albel727 albel727 closed this Jun 20, 2017
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.

1 participant