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

Split out low level interface to a separate dbus-sys crate #85

Closed
albel727 opened this issue Jun 8, 2017 · 29 comments
Closed

Split out low level interface to a separate dbus-sys crate #85

albel727 opened this issue Jun 8, 2017 · 29 comments

Comments

@albel727
Copy link
Contributor

albel727 commented Jun 8, 2017

This would be useful for separation of concerns and people will be able to build upon their own high level interface on the bindings.

Thing is, there are some inconveniences stemming from the current design, like the inability to handle MethodCall/Error from the iter(), etc.

Reliance on register_object_path() to handle objects is also something that can be avoided, because dbus_connection_register_object_path() and friends are just helper methods that basically create a BinarySearchTree<Path, Callback> (modulo fallback handling). User can totally use dbus_connection_add_filter() callbacks and implement own path dispatching instead.

The callback you unconditionally install with dbus_connection_add_filter() for the sake of iter() precludes the simplest "one callback for everything" handling that user might want, and you don't provide a high level function for installing additional user filters, that could bypass this limitation by allowing to handle messages before they're swallowed by pending_items queue or silently discarded altogether (like Errors or unregistered MethodCall-s). The Connection::msg_handlers() interface doesn't help with that in the slightest.

This is particularly inconvenient if user enabled eavesdrop on some messages, because then your object abstraction that relies on dbus_connection_register_object_path() begins to leak, since the latter doesn't check the destination field at all, so your objects begin to receive method calls destined for others as long as the path matches, and what's worse - answering them, resulting in multiple replies on the bus (from you and from the original destination).

You also don't seem to handle Error replies at all. In the case of Connection::send_with_reply() that implies a handler leak. A MessageReply instance will hang in self.handlers array forever, if the call fails, even if an Error with corresponding reply_serial is received.

All in all, I'm having a hard time wrapping my head over your library, so I would really appreciate if the extern definitions and message serialization/deserialization code would be split out to separate crates, so that one could build upon them.

@diwic
Copy link
Owner

diwic commented Jun 8, 2017

Split out low level interface to a separate dbus-sys crate

I'm okay with splitting out the ffi module to a dbus-sys crate. The reason for not doing it is just a historic remnant. Feel free to submit a PR.

dbus_connection_register_object_path() and friends are just helper methods
User can totally use dbus_connection_add_filter() callbacks and implement own path dispatching instead.

But if so, what is it that lists all registered paths for a peer? (I e, what you see with D-feet or other introspection tools when you click on a peer)

You also don't seem to handle Error replies at all.

I believe they work with send_with_reply_and_block. Maybe I haven't tested it properly with send_with_reply and send? Would something like this solve the problem for you:

@@ -130,6 +130,10 @@ extern "C" fn filter_message_cb(conn: *mut ffi::DBusConnection, msg: *mut ffi::D
             i.pending_items.borrow_mut().push_back(ConnectionItem::MethodReturn(m));
             ffi::DBusHandlerResult::NotYetHandled
         }
+        ffi::DBusMessageType::Error => {
+            i.pending_items.borrow_mut().push_back(ConnectionItem::MethodReturn(m));
+            ffi::DBusHandlerResult::NotYetHandled
+        }
         _ => ffi::DBusHandlerResult::NotYetHandled,
     };

You then check if you got an error back using the as_result method on the method return message.

allowing to handle messages before they're swallowed by pending_items queue

The reason callbacks are minimal (i e, everything just goes into the pending_items queue), is because of panic handling. I want panics to properly propagate to whatever called the dispatch function. The design is partially historic as well, as catch_unwind is stable now, which opens up other possibilities.

because then your object abstraction that relies on dbus_connection_register_object_path() begins to leak,

Is this the tree module that you're talking about? It's true that I didn't consider eavesdropping when I wrote that.

All in all, I'm having a hard time wrapping my head over your library

Do you think it's broken beyond repair, or would you like to help out with fixing the things you see as problematic? I currently don't have that much time so a helping hand would be welcome.

@albel727
Copy link
Contributor Author

albel727 commented Jun 8, 2017

But if so, what is it that lists all registered paths for a peer? (I e, what you see with D-feet or other introspection tools when you click on a peer)

Ah, yeah, that's an important nuance, indeed. That would be the courtesy of Introspectable interface, i.e. <node name='subpath'> elements in the XML returned on Introspect call, which every object is free to not implement, in which case libdbus will provide a default response based on paths registered with dbus_connection_register_object_path(), indeed. See the handle_default_introspect_and_unlock() function, which is called from the dispatch function if none of your handlers handle Introspect call.

So if you implement Introspectable on your object paths manually, you no longer need dbus_connection_register_object_path() at all. You can check dbus-connection.c and dbus-object-tree.c to be sure of that, in case I'm missing anything, but grepping shows to me that connection->objects (the binary search tree registered with the connection) is ultimately only used for dispatching and default introspection.

I believe they work with send_with_reply_and_block

Well, dbus_connection_send_with_reply_and_block() literally stops the event loop until a reply or timeout, so the replies to the call never even pass through the add_filter() callbacks.

I'm also not sure if send_with_reply() method isn't fundamentally suffering from race conditions, e.g. what if the response to send() comes and someone consumes it with iter() before you install MessageReply handler to msg_handlers() array, etc.

Would something like this solve the problem for you

Well, as I've said I haven't really wrapped my head over your library, so I'm not really sure how that will interact with more complicated aspects of it, like the tree module, but exposing Errors as MethodReturns to iter() callers might break current library users which might not have expected anything but well-formed response messages there. On the other hand, extending ConnectionItem enum with an Error variant would either break compilation due to failing exhaustiveness checks or unexpectedly bypass the previously used Nothing handling, going to the default match branch instead, so it's only marginally better.

It's true that I didn't consider eavesdropping when I wrote that.

Yeah, I wouldn't blame anyone here. It's a rare enough use case and it's probably as unexpected to everyone as it was to me that dispatching to callbacks registered with dbus_connection_register_object_path() wouldn't filter messages by destination at all.

This could be worked around if there was a way to install own filter in the chain before filter_message_cb() default handling, so that eavesdropped messages regardless of type could be handled and stopped before any other handling. Maybe indeed filter_message_cb() should be rewritten with catch_unwind() around a loop on closures registered with the connection with some high-level Connection::add_filter() call, and/or iter() call should install its own filter on demand only.

Do you think it's broken beyond repair, or would you like to help out with fixing the things you see as problematic? I currently don't have that much time so a helping hand would be welcome.

I'm very sorry to say, that I do not have a lot of free time either. To really be able to offer any help I will have to understand your library much more deeply that I currently do. As with the case with Errors above, I'm not sure everything be fixed without breaking backward compatibility either.

If I started from scratch, I'd probably based the library on a bare-bones dbus-sys core + tokio on top of that + high level object interface on top of that. But I don't feel like I know tokio and libdbus threading behavior in just enough detail yet. I regret it much, but I probably won't be able to offer you any help any time soon, I'm afraid.

On the upside, my current use case can be accomplished with your library as is, albeit in a somewhat hacky way, so there's no pressing need for you to do anything really. :)

@diwic
Copy link
Owner

diwic commented Jun 9, 2017

I'm also not sure if send_with_reply() method isn't fundamentally suffering from race conditions, e.g. what if the response to send() comes and someone consumes it with iter() before you install MessageReply handler to msg_handlers() array, etc.

As long as you don't call iter before you've added the handler to the msg_handlers array, you should be fine. The only thing that can call my callbacks is when I call into the FFI library.

So if you implement Introspectable on your object paths manually, you no longer need dbus_connection_register_object_path() at all.

Makes sense.

If I started from scratch, I'd probably based the library on a bare-bones dbus-sys core + tokio on top of that + high level object interface on top of that.

Okay. Well, we need a non-tokio version of the dbus bindings too, and then we need them to share as much code as possible. So for me the more logical way is to build the dbus-tokio version on top of the dbus library. I assume you've seen that I've started (if not, see the async directory), but due to lack of time (and tokio understanding) I'm not that far yet.

Or we could go the full way and get rid of the libdbus-1 dependency (and that library's peculiarities) altogether...

@albel727
Copy link
Contributor Author

albel727 commented Jun 10, 2017

As long as you don't call iter

Yeah, which is an artificial restriction, which is not documented anywhere and which precludes reliable use of multiple threads. Normally I would've expected the compiler to somehow prevent me from using a library in a way that would leak memory, which is bad for long-running services, such as what DBus would be often used in.

The DBus's native analog of this functionality, dbus_connection_send_with_reply(), does it the sorta-proper way with the analog of MessageReply called DBusPendingCall, which they add to the top of the filter chain atomically themselves, so there's no reliance on user managing to "add it to the chain fast enough". I call this "sorta-proper" because even though it doesn't leak memory at least, blocking is a bad solution.

Okay. Well, we need a non-tokio version of the dbus bindings too

My opinion is that because blocking is a bad solution in general, I wouldn't expose most of the libdbus calls to user in anything but the raw form. The really useful parts of the library when you're writing a client are connection_open/close(), DBusMessage serialization methods, send() and add_filter() + a few getters on DBusConnection, like get_unique_name(). The rest are either server-mode methods or helper methods that can be implemented in terms of the above.

I would have wrapped just this functionality in Tokio. If user wants "simple blocking calls", you can build them on top of tokio + wait(). If the user wants "full control", they're free to shoot themselves in the foot however they wish with the raw dbus-sys bindings. And if the user wants high-level "Remote Object" abstraction library, it can be implemented as a Tokio service. Tokio is exactly the right middle-level abstraction for DBus, I think.

So for me the more logical way is to build the dbus-tokio version on top of the dbus library.

Except for that task you'd only ever really need your Message parsing code + set_watch_functions() + set_timeout_functions() raw methods + the raw methods I outlined above, i.e. what I'd call dbus-sys. You certainly wouldn't need the high-level tree stuff and in fact I think it would live very well on top of Tokio. iter() could be just a futures::stream::Stream::wait(). I would also have thrown away dbus_connection_register_object_path() usage altogether and just handled everything in the filter_message_cb(), since you generate introspection data yourself anyway. So we might be violently agreeing here.

I assume you've seen that I've started (if not, see the async directory), but due to lack of time (and tokio understanding) I'm not that far yet.

Yeah, saw that too. I'm not exactly proficient with Tokio either, so I'm not sure at the moment how the set_timeout_functions() part of custom event looping can be handled there cleanly.

Or we could go the full way and get rid of the libdbus-1 dependency (and that library's peculiarities) altogether...

Funny you'd say that. I had exactly the idea! But having investigated the matter a bit, I conclude that this endeavor is not for the faint of heart. Libdbus handles quite a lot of things for you, that you as a user never have to think about. You use dbus_get_private(bus_type) helper call, which means you are able to gloss over these steps:

  1. Properly extract bus addresses from environment
  2. Parse each address to transport + setting keys
  3. Try connecting in turn with each of specified transports (Unix domain sockets/TCP, etc)
  4. Try to authenticate (SASL with several mechanisms, e.g. you may need to pass credentials/UID in the side-channel to the unix domain socket, or maybe some form of challenge/response with accessing a security token file, etc).
  5. The above also includes negotiating the ability to pass file descriptors via DBus messages (which is only possible via unix domain sockets at the moment).

Finally, with any luck you're connected, so now you see dbus messages coming at you. Now you need to implement your own message de/serialization, which means

  1. Handling both big AND little endian messages, which would be a lot of fun to try to handle in "as much zero-copy as possible" manner.
  2. De/serializing fields obeying very nontrivial alignment rules.
  3. Ensuring that mandatory fields in the message header are present depending on the message type, etc.
  4. Properly behaving with regard to no-reply and other flags in incoming message's header.
  5. Obeying various size limits of DBus specs.
  6. Implementing file descriptor passing, which again means unix domain socket side-channeling, something that not even nix, let alone mio/tokio is able to handle properly at the moment (Allow receiving multiple FD over ControlMessage in unix sockets nix-rust/nix#473). And I'm not even sure if/how libdbus passes more than 253 descriptors per message.
  7. Handling all that in secure manner, properly rejecting incoming malformed/oversized/out-of-spec messages. Descriptor passing can complicate that goal in funny and unexpected ways too, e.g. https://bugs.freedesktop.org/show_bug.cgi?id=80163

I wouldn't say that a simplistic no-fd-passing no-zero-copy client-side-only dbus library isn't possible. E.g. Rubyists managed to implement one. https://github.com/mvidner/ruby-dbus though its conformity to the spec is questionable. But it's gonna prove to be hard, and then libdbus may or may not incorporate some further security/performance fixes in the future, maybe kdbus will rise from the dead, maybe the spec will get updated, etc. Not to forget that Rust can't really handle failing memory allocations yet, so libdbus is fundamentally more robust in this regard.

I would honestly applaud to the effort should anyone attempt it.

@diwic
Copy link
Owner

diwic commented Jun 10, 2017

Yeah, which is an artificial restriction, which is not documented anywhere and which precludes reliable use of multiple threads.

Connection is not Sync, so not sure what the multiple threads you're talking about is referring to.

My opinion is that because blocking is a bad solution in general

I'm not saying you're wrong, but basic Rust design disagrees - with std being blocking I/O, and Tokio being a separate crate. So me doing the same would at least be consistent.

Also, I'm not sure Tokio is going to be the long time winner of Async I/Os. I find the design quite hard to grok compared to other event loops I've used in other languages. (You can't even start listening to an fd without depending on mio, which is going to be problematic when they try to write Tokio on top of glib!)
I would therefore prefer if the dbus crate to provide API points (Watch fd, MsgHandler etc), and for Tokio and potentially other Async I/O libraries to use them.

this endeavor is not for the faint of heart.

But for the person who has a strong heart and lots of time, I think Rust would be a good language to write such a library in :-)

maybe kdbus will rise from the dead

Actually, its successor is BUS-1. Let's see what happens with that.

@diwic
Copy link
Owner

diwic commented Jun 11, 2017

Except for that task you'd only ever really need your Message parsing code + set_watch_functions() + set_timeout_functions() raw methods + the raw methods I outlined above, i.e. what I'd call dbus-sys.

Hmm, I believe this is approximately the parts of Connection that dbus-tokio would end up using.

Also, I've heard your objection about register_object_path vs filter, will look into that when/if I get time.

Libdbus handles quite a lot of things for you

Btw, makes me wonder if it would be possible to use a half-measure so that the dbus library is used for connecting, but then you just grab its fd and do all writing and reading yourself. That way you would only get the second half of the problems you're describing.

Also, for bus1, I believe the intention is to create a backwards compatibility layer on socket level, i e, if libdbus were to be rewritten in Rust, it would still work with bus1. Hopefully...

@albel727
Copy link
Contributor Author

Connection is not Sync, so not sure what the multiple threads you're talking about is referring to.

Ah, I didn't notice that IConnection contains Cells. Alright, so the compiler does prevent one from using the library with multiple threads. Is that really a good thing? A legit question. It may be, if DBus is somehow less stable when used by multiple threads, and apparently there were complaints like that in the past. Though I'd expect most bugs to be ironed out at this point.

But anyway, should the iter()/msg_handlers() ordering thing really lay waiting to trip user up, however unlikely that is? I would just put MessageReply into msg_handlers() inside send_and_reply(). Better still, how about making MessageReply wrap DBusPendingCall, which does handle this stuff properly? It would probably be the best solution, as it is atomic, more scalable as it internally uses a hash map by reply_serial instead of a linear pass through msg_handlers() array, and it would fix the memory leak without needing to apply the backward incompatible patch that wraps Errors in ConnectionItem::MethodReturns.

You know what, let me put my code where my mouth is... done. Here's a PR. #86

I find the design quite hard to grok compared to other event loops I've used in other languages.

Well, maybe the event loop itself isn't much different, but Rust has quite peculiar futures, and that affects everything.

I would therefore prefer if the dbus crate to provide API points (Watch fd, MsgHandler etc), and for Tokio and potentially other Async I/O libraries to use them.

Well MsgHandler is of limited utility as it stands. The unconditionally bolted-on iter() that ignores half of message types makes it an irregularity to work around rather than anything helpful in my case. If instead messages of all types were put in the queue and/or Connection had a customizable callback setup, it would have been much nicer.

Also, I've heard your objection about register_object_path vs filter, will look into that when/if I get time.

That's nice to hear. It might be that I also will find time and try doing something about it, then.

Btw, makes me wonder if it would be possible to use a half-measure so that the dbus library is used for connecting, but then you just grab its fd and do all writing and reading yourself.

I rather think that the connect/auth part is much more easier than the rest. So I'd rather try using libdbus for all the message de/serialization thing instead. IIRC there are methods that allow parsing a dbus message from/into byte array, at least. The headache with adhering to the protocol and fd passing would be there still, though. But, at least, I looked at the alignments/endianness thing again, and I think it wouldn't be all that hard to implement it on par or even better than libdbus.

Also, for bus1, I believe the intention is to create a backwards compatibility layer on socket level, i e, if libdbus were to be rewritten in Rust, it would still work with bus1.

Well, from what I read, I can only imagine it working as an alternative faster transport, i.e. an alternative to UDS/TCP. But it's quite likely that it would be just an opt-in, so older libraries would still be able to work via UDS/TCP unmodified.

@diwic
Copy link
Owner

diwic commented Jun 14, 2017

Also, I've heard your objection about register_object_path vs filter, will look into that when/if I get time.

That's nice to hear. It might be that I also will find time and try doing something about it, then.

I've just pushed a few commits for this. Let me know if you like it. :-) Maybe I should also add a switch that always returns "Handled" from the filter, which would entirely disable the default handler(s) if any.

@diwic
Copy link
Owner

diwic commented Jun 14, 2017

Well MsgHandler is of limited utility as it stands. The unconditionally bolted-on iter() that ignores half of message types makes it an irregularity to work around rather than anything helpful in my case. If instead messages of all types were put in the queue

This should now be fixed in git master.

and/or Connection had a customizable callback setup, it would have been much nicer.

Well, Tree and MsgHandler are my two attempts at a customizable callback setup. What other type of callback setup would you suggest? MsgHandler is quite new and not many people depend on it, I think I could do a redesign and release v0.6 without anyone complaining much.

Edit: Answering my own question so you don't have to: you suggested to

  • put the MsgHandlers on Connection instead of ConnectionItem
  • have some type of timeout so that they don't stay in there forever in case there is no reply
  • automatically add the MessageReply on the Connection when calling send_and_reply

Did I miss anything?

@albel727
Copy link
Contributor Author

I've just pushed a few commits for this. Let me know if you like it. :-)

Yeah, those are great. Makes for a clearer mental model, at least for me, though users may still be tripped up by the fact that ConnectionItem::MethodReturn can actually contain not just the DBus Spec definition of METHOD_RETURN messages, but also ERROR messages as well. Maybe this should be documented more explicitly.

Maybe I should also add a switch that always returns "Handled" from the filter, which would entirely disable the default handler(s) if any.

You mean something like Connection::set_handle_everything(bool) so that filter_message_cb() would always return Handled? Though I had more elaborate setups in mind (described below), adding a simple boolean switch field to Connection is a useful-enough alternative, yeah.

The only default handling in libdbus I see is

  1. The poor man's object path introspection I've mentioned before.
  2. The auto-reply with "invalid object path"/"invalid object method" error messages if you returned NotHandled for a METHOD_CALL.

Of those only the latter seems like it could be missed, as Introspectable is optional, but "one and only one reply per method call" sounds like a sort of guarantee that better be upheld reliably.

Funnily enough, I can't seem to find the logic that prevents auto replying to NO_REPLY_EXPECTED METHOD_CALLs. Maybe filtering out unsolicited replies is built-in in the dbus daemon itself. After all it does seem to have some auto-reply logic that guarantees at least one reply by issuing a "timeout error" to the caller if the callee never replied. Oh well.

Anyway, a simple "Sit back, I know what I'm doing" switch might be nice still.

This should now be fixed in git master.

The git changes are very helpful indeed. But the bolted-on aspect of iter() is kind of there still, as there's no way to add your own filters that could override Handled/NotHandled response of the queue pusher callback, or disable the queue pusher altogether.

The latter might be made possible with a simple boolean switch. Alternatively, one can register the queue pusher callback only if the user calls iter() at least once. There are downsides to the latter design, though, e.g. messages might be missed if you don't add the callback fast enough, etc. Maybe this whole "customizable callback" thing would live better as a separate CustomConnection type or something, so that it doesn't interfere with the current Connection design. I need to think that one through and I don't have time for it.

customizable callback setup

Ah, that referred to the idea of exposing some refined version of dbus_connection_add/remove_filter() to the user, so that they could freely catch everything they choose and decide whether to respond with Handled/NotHandled (or maybe even NeedsMemory, in the "Rust with fallible allocations" future) however they want, and also completely bypass the default logic that just catches everything and pushes to a queue, which might be redundant for some use cases.

For example, there's that little snag with the current design that, while eavesdropping, one has to call not only add_match(path=the_path) but also register_object_path(the_path) so that Handled is returned to libdbus in order to prevent it from generating an auto-response with the "invalid object path" error on every eavesdropped METHOD_CALL. Processing eavesdropped messages with a separate compact handler returning Handled might be of better ergonomics than register_object_path() + filtering out "redundant" METHOD_CALLs somewhere in the middle of iter() loop.

One possible design could be, yeah, making msg_handlers() be a property of Connection and processing msg_handlers() not in iter() but directly in filter_message_cb(), returning Handled based on their result, and only then falling back to the 'push to queue' logic.

For an alternative, lower-level design, imagine

pub struct MsgFilter { 
  handler: extern "C" fn(*mut ffi::DBusConnection, *mut ffi::DBusMessage, *mut c_void) -> HandlerResult, 
  user_data: *mut c_void,
  free_user_data: extern "C" fn(*mut c_void) 
}

Then there would be Connection::add/remove_filter<F: Into<MsgFilter>>(&mut self, f: F) which would call the corresponding libdbus functions. And then there would also be, e.g. impl From<T> for MsgFilter where T: FnOnce(Connection, Message), etc, so that user doesn't need to ever write the C callbacks themselves. Lifetimes/ownerships might need to be carefully considered though.

Did I miss anything?

I don't think so. Though the whole design needs to be considered carefully, as well as the whole question whether you really want to reimplement everything that libdbus supports and guarantees already.

E.g. for send_with_reply() you want to guarantee that the given callback will be called for the reply message and that nobody else should see/intercept the reply, otherwise user would've just used the regular send() and handled the reply in iter() directly. So putting awaited replies to the pending_items queue is redundant and simply putting the MessageReply handler to msg_handlers() is probably wrong because you shouldn't let some other handler from msg_handlers() accidentally consume the reply instead of the user's callback. These requirements imply, e.g. the "hash map by reply_serial that is consulted before msg_handlers() altogether" design, like in libdbus.

Timeouts is another can of worms. Even if you reimplement them yourself for send_with_reply(), I think there are other sorts of timeouts in libdbus, that you won't be able to cover without using set_timeout_functions() anyway, and not covering which might open your app for problems, like connection/auth timeouts.

@diwic
Copy link
Owner

diwic commented Jun 17, 2017

So, when the queue push thing was implemented, catch_unwind was not stable, so therefore I wanted the C callback as short and deterministic as possible, so that it couldn't possibly panic (because that would unwind us into C, which is undefined behaviour).

I like the idea of having a callback that could determine whether or not to call libdbus's fallback methods. We just need to carefully consider what should happen in case this callback panics. Which is now possible to handle gracefully, now that catch_unwind is stable.

like connection/auth timeouts.

This sounds like something handled inside dbus_bus_get_private - no need for timeout functions?

@albel727
Copy link
Contributor Author

We just need to carefully consider what should happen in case this callback panics.

Yeah. Well, assuming we manage to make handler loop UnwindSafe, the first thing to try is just close the connection. I don't think other coping strategies are really valid here. I'm not sure right now, whether it's allowed from inside the filter callback, and whether it's allowed to call close() twice or we should also add a boolean field to the struct so that Drop doesn't do it again, etc. Don't have time to check sources, maybe later.

This sounds like something handled inside dbus_bus_get_private - no need for timeout functions?

Right. I've seen the libdbus server code using timeouts for auth/connection, and assumed client code would do the same. So, now I see, libdbus client is blocking during connection. That's a bit sad.

@diwic
Copy link
Owner

diwic commented Jun 18, 2017

What do you think: 4f2870d

Not sure why we want to close the connection in case of panic?

@albel727
Copy link
Contributor Author

Not sure why we want to close the connection in case of panic?

That was compared to other more lax approaches like skipping failed handler (I imagined there would be a Vec of them) or skipping failed message altogether. Bubbling the panic up to iter() caller is fine too.

What do you think

Looks fine. Though I wonder if it'd be better to add UnwindSafe bound to filter_cb, and make the user guarantee it, instead of asserting safety over unknown user code.

Now, though, since there isn't a Vec of handlers (which may or may not be a good thing really, considering the necessary dances like the one you did with msg_handlers() to work around modifying-while-iterating problem), the user is not able to stack functionality, unless they manually call the previous handler in chain. And that they cannot do. The filter_cb field is private, I don't see a getter, and default_filter_callback() is private too, so the user isn't able to install their own handler and let iter() receive the rest.

More troubling still, is that they're unable to use their custom handler at all without calling iter(), and once they do that, there's no way to break out of next(), if the custom handler lets nothing past itself and into pending_queue. And I don't think it really should be mandatory to put stuff into the queue in order to break out of the loop.

If there would be a looping construct alternative to iter() that would solve the above problem, by basically exposing some form of read_write_dispatch(), the access to the now private filter_cb_panic might be needed too.

@diwic
Copy link
Owner

diwic commented Jun 19, 2017

Though I wonder if it'd be better to add UnwindSafe bound to filter_cb, and make the user guarantee it, instead of asserting safety over unknown user code.

Since the unwind is resumed, I don't think this is necessary? I e, unless the user catches the panic there is no way for the user to observe the potential broken invariants inside the user's filter_cb.

The filter_cb field is private, I don't see a getter, and default_filter_callback() is private too, so the user isn't able to install their own handler and let iter() receive the rest.

I'm thinking that making default_filter_callback public would be the most appropriate here. That way a user can also temporarily switch to another callback, and then switch back to default_filter_callback when done, which could be useful.

More troubling still, is that they're unable to use their custom handler at all without calling iter(), and once they do that, there's no way to break out of next(), if the custom handler lets nothing past itself and into pending_queue.

If you call iter() it will return a Some(Nothing) after timeout_ms (which can be zero), if the filter callback puts nothing in it. So it will work, but some looping construct might be more ergonomic.

@albel727
Copy link
Contributor Author

I e, unless the user catches the panic there is no way for the user to observe the potential broken invariants inside the user's filter_cb.

Yeah, it isn't necessary. I just thought that they might be catching it to guard against some other processing, never expecting iter() to panic or not realizing that not just the connection but what the callback closes over will be affected too. UnwindSafe bound on the callback would make the requirement more apparent, though maybe more inconvenient to the user, as it would probably force them to AssertUnwindSafe themselves, even if they never intended to catch any panics. But if they catch the panic and unknowingly try to reuse the connection, the once failed callback and things it closes over may subtly fail, which is another reason why closing the connection altogether was on my mind. But then, after all, it's their fault for messing with catch_unwind() around iter() and they also would have had to assert unwind safety on either Connection or the callback for it to compile, so your current approach is probably the sanest.

I'm thinking that making default_filter_callback public would be the most appropriate here.

I, on the contrary, think that providing the getter would be most general, though making default_filter_callback() public wouldn't hurt either. The default callback can be obtained by simply querying the Connection after creation, but no getter means you have at most one level of indirection if handlers aren't aware of each other. Imagine a library crate that installs own callback on a given connection, and then you want to install your own callback on top of that. Or worse still, with a library needing to be the last to install a handler. With the getter it's simply "get the current handler whatever it is and install your own handler that calls it". Without, it's a case-by-case exercise, and that's hoping that libraries are written with their handlers made public too or accept your handler to chain into, or it's not doable at all.

If you call iter() it will return a Some(Nothing) after timeout_ms (which can be zero)

Right. Of course it does, I'm not sure how could this have slipped my mind. I always write my responses while being too tired to think, it seems, and now I missed the negation before the queue emptiness check while rereading the code of iter(), even though I knew how iter() functions from experience.

@albel727
Copy link
Contributor Author

By the way, what's the point of DBusCallback type, and why don't you use DBusHandleMessageFunction instead? It's only used once in an unnecessary filter_message_cb as DBusCallback cast, and if there's some deep significance to that, I fail to see it. It's just that I'm slowly trying to shape out a dbus-sys crate here, and so this is a candidate for removal.

@albel727
Copy link
Contributor Author

What do you think? #87

@diwic diwic closed this as completed in 7ccb33c Jun 20, 2017
@diwic
Copy link
Owner

diwic commented Jun 20, 2017

@albel727
I think your dbus-sys crate is good work, so I merged it! The only thing I wondered a bit about whether we actually needed to duplicate WatchEvent, but probably this was just me doing something slightly silly at some point so probably you had no better option.

By the way, what's the point of DBusCallback type, and why don't you use DBusHandleMessageFunction instead?

No idea, so I like your suggestion to remove it :-)

I, on the contrary, think that providing the getter would be most general, though making default_filter_callback() public wouldn't hurt either.

Hmm. So I made a change that made set_message_callback return the old callback. I think that's the closest to a getter that we get? At least if we allow closures and not just "free functions without environmen", I mean - how do you make a (useful) getter to a closure?

Right. Of course it does, I'm not sure how could this have slipped my mind. I always write my responses while being too tired to think,

FWIW, your hitrate (i e rate of correct responses compared to missed things) seems pretty good to me :-)

@albel727
Copy link
Contributor Author

The only thing I wondered a bit about whether we actually needed to duplicate WatchEvent

Maybe we didn't. Thing is,

  1. WatchEvent::from_revents() is a helper method that doesn't entirely belong to a raw bindings crate.
  2. What it does is poll() specific, and libdbus can work on Windows and with other event loops too.
  3. It depends on (unix-specific part of) libc, and I wanted to avoid depending on libc for such a minor thing.

So I decided against including this method in libdbus-sys. But one can't impl WatchEvent { fn from_revents() {}} from outside the crate that defines WatchEvent type, so I couldn't leave it as it was either. And since it is public API, I couldn't get rid of it altogether or refactor it to be a top-level method, etc.

I could add a new empty struct WatchEvent and implement from_revents() on that, and make all references to enum variants in code qualified by DBusWatchEvent:: instead of WatchEvent::. But they're used in quite a few places, including the async stuff you lately work on, and so I thought this renaming patch noise wouldn't be the best idea.

So I decided to simply copy the enum, since the libdbus constants are quite unlikely to change ever now, so no harm from duplication is expected. In fact the first thing I wrote was like

pub enum WatchEvent {
    Readable = ffi::DBusWatchFlags::Readable as isize,
    Writable = ffi::DBusWatchFlags::Writable as isize,
    Error = ffi::DBusWatchFlags::Error as isize,
    Hangup = ffi::DBusWatchFlags::Hangup as isize,
}

But then I wondered if all stable compilers support this form of initialization, and wrote the simple constant version instead.

I also took this opportunity to rename DBusWatchEvent to DBusWatchFlags like they're called in libdbus, and refactor WatchEvent out of connection.rs and into watch.rs, which feels like the place it belongs to.

I guess I can move it into libdbus-sys, and qualify by #[cfg(unix)], just like the stuff it depends on in libc, if you insist. But I think the job is better done by users that want poll() themselves and using some kind of EnumMap/EnumSet or something for actual values.

Hmm. So I made a change that made set_message_callback return the old callback. I think that's the closest to a getter that we get?

Well, in my imagination it worked like that. There would be take_message_callback() -> Option<Box<FnMut(&Connection, Message) -> bool>> that would be like Option::take(), i.e. putting None instead. It's a matter of taste whether None would mean "always NotHandled" or panic, if the user attempts to run stuff without setting own callback, but you already do the panic, so let it be so (although the panic message may need to be altered). And then one can do

let old_cb = c.take_message_callback();
c.set_message_callback(|c, m| {
  if m == what_i_want {
    //do stuff
    return true;
  }
  old_cb.map(|f| f(c, m)).unwrap_or_default()
});
Library::install_another_callback(&c);
//finally run it
for ci in c.iter() { ... }

One can somewhat emulate take_message_callback() with set_message_callback(|_,_| { false }) though, so I guess it's sufficient.

@diwic
Copy link
Owner

diwic commented Jun 21, 2017

Hmm. I was wondering if there should in fact not be a WatchFlags enum in dbus-sys, but free constants instead? Since they're flags that can be combined. Otherwise I share your analysis.

There would be take_message_callback()

Ah, so you meant to take rather than get. I guess one could also do something like:

let old_cb = Rc::new(RefCell::new(Box::new(|_,_| { false }))); // Dummy function, soon to be replaced
let old_cb2 = old_cb.clone();
*old_cb2.borrow_mut() = c.set_message_callback(move |c, m| {
    // do stuff
    let old_f = old_cb.borrow_mut();
    old_f(c,m)
});

...but I don't know if it would be better, really.

@albel727
Copy link
Contributor Author

I was wondering if there should in fact not be a WatchFlags enum in dbus-sys, but free constants instead? Since they're flags that can be combined.

Well, the same applies to DBusNameFlag, for that matter. It's public API too, should we make a copy like with DBusWatchFlags, in order to fix it? Funnily enough, DBusNameFlag constants actually are defined in libdbus as a set of defines like DBUS_NAME_FLAG_REPLACE_EXISTING, whereas DBusWatchFlags, on the contrary, is defined as enum there too.

I guess flag enums can be converted, e.g. to modules with consts in them, but I'm not sure there's much benefit to it. I don't expect there would be confusion about the fact that these enums don't strictly function as Rust enums, but as convenient groupings for constants. Using enums for flags is familiar enough that people are likely to implement something like #[derive(EnumSet)] that would produce a set type given an enum with flags. Same functionality with a module that happens to contain only consts is harder to imagine.

On the other hand, it might be inconvenient to cast enum constants to ints everywhere. So, I'm not sure on the matter. Maybe DBusWatchFlags should be converted to top-level constants with names like DBUS_WATCH_FLAG_READABLE. Maybe it would be mod dbus_watch_flags { const READABLE = 1; ... }. Maybe DBUS_TYPE and DBUS_TIMEOUT groups of constants should be turned to enums or const modules too, for that matter.

I'll leave the decision to you. What do you think we should do here?

Ah, so you meant to take rather than get.

Yeah, I understand a literal getter there would be hard to come by, unless one would change filter_cb to be an Rc or do something else that isn't worth it for a simple filter callback with a getter.

...but I don't know if it would be better, really.

Yeah, that works too, sans a missing .unwrap(). Though twice calling set_message_callback() makes the intent more immediately clear to me. Either way, having to allocate a temporary empty closure on the heap feels sort of roundabout, but that much can be lived with, I guess. :)

@diwic
Copy link
Owner

diwic commented Jun 22, 2017

I'll leave the decision to you. What do you think we should do here?

Ok, so I changed dbus-sys to what I think is better, and left it the way it was in dbus for backwards compatibility.

Either way, having to allocate a temporary empty closure on the heap feels sort of roundabout, but that much can be lived with, I guess. :)

let old_cb = Rc::new(RefCell::new(None))); 
let old_cb2 = old_cb.clone();
*old_cb2.borrow_mut() = Some(c.set_message_callback(move |c, m| {
    // do stuff
    let old_f = old_cb.borrow_mut().as_mut().unwrap();
    old_f(c,m)
}));

But it's still ugly. Hmm, what if we add take_message_callback as sugar for set_message_callback(default_filter_cb)?

@albel727
Copy link
Contributor Author

albel727 commented Jun 23, 2017

AllowReplacement = ffi::DBUS_NAME_FLAG_ALLOW_REPLACEMENT as isize,

Might as well do the same with WatchEvent then?

Some(c.set_message_callback(

set_message_callback already returns an Option though? Anyway, I get the idea. It would work, I guess. Though technically Rc<RefCell<>> means a heap allocation for the reference counters storage and a counter check + a borrow check for each access to the closure inside the callback.

Hmm, what if we add take_message_callback as sugar for set_message_callback(default_filter_cb)

I think it's rather pointless. It would needlessly expand API surface, just to save a few keystrokes, while still making a Box allocation and having nontrivial side-effects instead of failing fast, if the user mistakes it for a true getter and/or accidentally calls it several times in a row/calls it again after setting own callback. It would mean a couple of confused minutes of debugging for the user to figure out why their application seems to almost function, seeing iter() producing items, while custom callbacks seeming to pass everything through/never get called.

To compare, putting None is better in those regards, since it doesn't need a heap allocation and, should it happen to be misused, trying to call iter().next() would immediately end the application with a panic, which, provided an appropriate panic message, would explain the problem rightaway. It would also allow to implement chaining without Rc<RefCell<>> overhead.

take_message_callback() not exactly functioning like Option::take() would probably be against user expectations too, so I'd rather not add it at all then.

@albel727
Copy link
Contributor Author

albel727 commented Jun 23, 2017

while still making a Box allocation

Though I begin to doubt that something will be allocated on the heap for plain function arguments, thanks to trait object and ZST optimizations. Oh well.

EDIT: Yeah, it wouldn't.

@diwic
Copy link
Owner

diwic commented Jun 23, 2017

Might as well do the same with WatchEvent then?

Yup. Done.

To compare, putting None is better

Thanks for the review. I've listened to your arguments and changed the code accordingly. Have a look. The message callback can now be unset. One now has to write an extra Some(Box::new( when replacing the message callback with another, but I think that's okay since it makes more clear what the function is actually doing.

@albel727
Copy link
Contributor Author

Have a look.

Great. replace_message_callback is a good alternative to having two methods. Though maybe change the type of filter_cb in IConnection to RefCell<Option<MessageCallback>> for even better code clarity, unless I have failed to see a reason not to? And maybe use MessageCallback as a proper noun in the error text? Since the general "a message callback" might be confused, say, with MessageReply, and MessageCallback has further associated documentation directly pointing to replace_message_callback(), which would be great for those who haven't used replace_message_callback() themselves but fell prey to someone else introducing the bug with it.

Here, to maybe save your time. #88

@diwic
Copy link
Owner

diwic commented Jun 24, 2017

👍 Merged.

Btw, are you happy with libdbus-sys as it looks now, i e, should I release a v0.1.0 of it?

@albel727
Copy link
Contributor Author

I think I am. Yeah, go ahead. I doubt the already existing stuff would change from now on, only some missing methods appended, which can well be a backward-compatible v0.1.1, if such need arises.

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

No branches or pull requests

2 participants