Improve mdb #96

kjughx · 2024-10-07T11:38:23Z

It doesn't make much sense to bundle OwnedOrBorrowedError when they serve different use cases, better split them into separate types.
Message borrows *const mdb_sys::mdb_message_t so should contain a marker with a lifetime bound.
The callbacks used in mdb are hidden behind pointers to pointers to DSTs when they could just be generic pointers. This also simplifies the UX since a consumer doesn't have to wrap callbacks in Box::new().

crates/mdb/src/lib.rs

apps/consume_analytics_metadata/src/main.rs

apljungquist · 2024-10-07T12:48:36Z

crates/mdb/src/error.rs

+
+pub struct BorrowedError<'a> {
+    ptr: *const mdb_sys::mdb_error_t,
+    _marker: PhantomData<&'a mdb_sys::mdb_error_t>,


Well spotted.

As I think more about this, I start doubting that BorrowedError should have an associated lifetime; maybe it is better/correct that the API only gives out &'a BorrowedError instead, where 'a is the lifetime of the reference. This looks at least superficially similar to &'a CStr.

I want to do some research on this as well; If you are impatient about getting the new callbacks merged, I would suggest scaling back the scope of the PR to only those.

See #116, there the API only exposes Error and &Error. The owned Error as a return value and &Error in the callback.

crates/mdb/src/error.rs

apljungquist · 2024-10-07T13:01:10Z

crates/mdb/src/error.rs

+    value
+}
+
+pub trait MdbError<Ptr> {


What are some benefits of defining a trait for this?

In particular, do we really want new and as_ref to be available to users of the library?

apljungquist · 2024-10-07T13:06:22Z

crates/mdb/src/lib.rs

-        write!(f, "Code: {}; Message: {:?};", self.code(), self.message())
-    }
-}
+pub trait Callback<T>: FnMut(T) + Send + 'static {}


Interesting, I have not seen this pattern for handling callbacks before. What are some benefits of using our own trait instead of relying on only the Fn* traits?

Have you seen any other crates or authors using it (i.e. is there anywhere I can learn more about it)?

The benefit of this is not having to repeat for<'a> FnMut(BorrowedError<'a>) + Send + 'static et. al. and risk making mistakes. I am still weary of introducing a trait for this purpose, so I will experiment a bit to learn more and maybe see if there is another way to achieve this DRYness.

apps/consume_analytics_metadata/src/main.rs

apljungquist · 2024-10-07T13:27:24Z

crates/mdb/src/lib.rs

-                }
-                (false, true) => {
-                    let on_message = config.on_message;
-                    config.on_message = std::ptr::null_mut();


If we don't store the on_message callback on the Subscriber, will it not be dropped as soon as this try_new returns?

Good point. Could Subscriber just own SubscriberConfig which then is dropped when Subscriber is dropped?

It could. I actually wrote it like that at first to minimize the complexity, but then I changed my mind thinking that it is unnecessary to tie up the mdb_subscriber_config_t for the lifetime of the connection. Are there reasons other than complexity for going back? Is there anything in particular that you think contributes to complexity with the current solution (assuming complexity is the reason you want to change this)?

crates/mdb/src/lib.rs

The variants of this enum are always treated as separate types so they should just be separate types.

Message borrows *const mdb_sys::mdb_message_t and should have a marker to indicate that.

It's very clunky to have to pass a Box<> callback. The functions that accept callbacks should be generic over the callback. The ownership of the callbacks was also unclear. Also no reason to allocate pointers to pointers on the heap. pointers are pointers.

apljungquist · 2024-10-09T16:28:01Z

Consider splitting the PR, the parts that are purely a refactoring are an especially good candidate. That way we can get something merged faster by focusing our attention on a smaller change. It may also help us write a good commit message; I'm trying to get into the habit of following conventional commits, but since I'm inconsistent at best this is not a requirement.

apljungquist · 2024-10-09T15:51:05Z

crates/mdb/src/lib.rs

-        write!(f, "Code: {}; Message: {:?};", self.code(), self.message())
-    }
-}
+pub trait Callback<T>: FnMut(T) + Send + 'static {}


The benefit of this is not having to repeat for<'a> FnMut(BorrowedError<'a>) + Send + 'static et. al. and risk making mistakes. I am still weary of introducing a trait for this purpose, so I will experiment a bit to learn more and maybe see if there is another way to achieve this DRYness.

crates/mdb/src/lib.rs

apljungquist · 2024-10-09T16:04:44Z

crates/mdb/src/lib.rs

-                }
-                (false, true) => {
-                    let on_message = config.on_message;
-                    config.on_message = std::ptr::null_mut();


It could. I actually wrote it like that at first to minimize the complexity, but then I changed my mind thinking that it is unnecessary to tie up the mdb_subscriber_config_t for the lifetime of the connection. Are there reasons other than complexity for going back? Is there anything in particular that you think contributes to complexity with the current solution (assuming complexity is the reason you want to change this)?

apljungquist · 2024-10-09T16:09:49Z

crates/mdb/src/error.rs

+
+pub struct BorrowedError<'a> {
+    ptr: *const mdb_sys::mdb_error_t,
+    _marker: PhantomData<&'a mdb_sys::mdb_error_t>,


As I think more about this, I start doubting that BorrowedError should have an associated lifetime; maybe it is better/correct that the API only gives out &'a BorrowedError instead, where 'a is the lifetime of the reference. This looks at least superficially similar to &'a CStr.

I want to do some research on this as well; If you are impatient about getting the new callbacks merged, I would suggest scaling back the scope of the PR to only those.

apljungquist · 2024-10-09T16:16:20Z

crates/mdb/src/lib.rs

 }

 impl Connection {
    // TODO: Consider adopting a builder-like pattern.
-    pub fn try_new(on_error: Option<Box<OnError>>) -> Result<Self, Error> {
+    /// Passing `None` as the `@on_error` callback requires qualifying the generic like so:


As far as I know it is convention in Rust to reference parameters without an @ prefix, see for instance Box::new.

kjughx force-pushed the mdb branch from 4299526 to 831cd26 Compare October 7, 2024 11:39

apljungquist reviewed Oct 7, 2024

View reviewed changes

Philip Johansson added 2 commits October 9, 2024 09:35

mdb: Split OwnedOrBorrowedError into separate structs

e94d19f

The variants of this enum are always treated as separate types so they should just be separate types.

mdb: Add marker for Message

36327af

Message borrows *const mdb_sys::mdb_message_t and should have a marker to indicate that.

kjughx force-pushed the mdb branch from 831cd26 to 5277318 Compare October 9, 2024 09:41

kjughx force-pushed the mdb branch from 5277318 to 9b35225 Compare October 9, 2024 12:46

apljungquist reviewed Oct 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve mdb #96

Improve mdb #96

kjughx commented Oct 7, 2024 •

edited

Loading

apljungquist Oct 7, 2024

apljungquist Oct 9, 2024

kjughx Oct 26, 2024

apljungquist Oct 7, 2024

apljungquist Oct 7, 2024

apljungquist Oct 9, 2024

apljungquist Oct 7, 2024

kjughx Oct 9, 2024

apljungquist Oct 9, 2024

apljungquist commented Oct 9, 2024

apljungquist Oct 9, 2024

apljungquist Oct 9, 2024

apljungquist Oct 9, 2024

apljungquist Oct 9, 2024

Improve mdb #96

Are you sure you want to change the base?

Improve mdb #96

Conversation

kjughx commented Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apljungquist commented Oct 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjughx commented Oct 7, 2024 •

edited

Loading