search

erickt / rust-zmq

Rust zeromq bindings.
Apache License 2.0
887 stars 189 forks source link

Message: Add pub unsafe fn `from_raw` #299

Closed kalcutter closed 1 year ago

kalcutter commented 4 years ago

Add Message::from_raw to create a Message from an initialized zmq_msg_t. This is useful for wrapping messages created with zmq_msg_init_data.

kalcutter commented 4 years ago

Maybe from_raw is not the best name since it typically refers to functions that take raw pointers. Maybe from_inner or from_msg? I am open to suggestions on the name.

kalcutter commented 4 years ago

I changed the name to from_msg.

rotty commented 4 years ago

Could you elaborate on the use-case for this new method? The reason why I'm asking is that currently, the existence of zmq_sys is an implementation detail, and not exposed to the user of the zmq crate, and I am a bit reluctant to change that.

Edit: I understand that this method is helpful in a mixed C/Rust codebase, where the C code happens to use zmq_msg_init_data, and wants to hand that to Rust for actually sending the message. I however wonder what circumstances give rise to this situation -- to me, it seems that when you have a mixed C/Rust codebase, and your "zmq handling code" is implemented in Rust, it is unlikely that you want to have code sitting "above it" (when thinking in layers) which is written in C.

kalcutter commented 4 years ago

Basically we have a Arc<dyn AsRef<[u8]> + Send> that we need to send zero-copy (performance is critical). The data could be memory-mapped in or be part of another existing data structure. Using zmq_msg_init_data we can reference the data and have the reference count decremented when the data has been sent.

I chose this API because I though it would be the most flexible including the added benefit to mixed C/Rust codebases (as you mentioned). This crate advertises itself as a binding for libzmq, so I think depending on types from libzmq is justifiable.

kalcutter commented 4 years ago

Also Socket provides similar functions from_raw and into_raw. So there is already a precedence for this sort of thing.

kalcutter commented 4 years ago

To keep zmq-sys an implementation detail, couldn't we just re-export zmq_msg_t under this crate?

rotty commented 4 years ago

Basically we have a Arc<dyn AsRef<[u8]> + Send> that we need to send zero-copy (performance is critical). The data could be memory-mapped in or be part of another existing data structure. Using zmq_msg_init_data we can reference the data and have the reference count decremented when the data has been sent.

OK, so what's actually missing in that instance is a binding to zmq_msg_init_data. I'd rather add that, instead of inviting users to dip down into zmq_sys territory.

Also Socket provides similar functions from_raw and into_raw. So there is already a precedence for this sort of thing.

Indeed. However, there's a difference here -- if you have a mixed Rust/C codebase, and need to pass zmq sockets from C to Rust (or in the other direction), you have to have these methods, while you can avoid the proposed from_msg method, at the cost of performance.

kalcutter commented 4 years ago

OK, so what's actually missing in that instance is a binding to zmq_msg_init_data. I'd rather add that, instead of inviting users to dip down into zmq_sys territory.

A more general binding to zmq_msg_init_data could be useful and allow some users to avoid zmq_sys. But what should such a binding look like? I have attempted to create such an API that would cover my use-case here: https://github.com/kalcutter/rust-zmq/commit/message-from_box_with. What are your thoughts on this?

Notwithstanding a binding to zmq_msg_init_data, I still think from_msg (this PR) is an elegant escape hatch for advanced use-cases (and should be merged :blush:).

Indeed. However, there's a difference here -- if you have a mixed Rust/C codebase, and need to pass zmq sockets from C to Rust (or in the other direction), you have to have these methods, while you can avoid the proposed from_msg method, at the cost of performance.

That may be a difference but is the distinction so important? The crate is acknowledging that Rust/C use-cases exist and should be supported (otherwise from_raw and into_raw would not exist on Socket). So if this is already the case, what is the harm in embracing a similar API on Message? Especially given that it is such a trivial change.

kalcutter commented 1 year ago

@Jasper-Bekkers Please consider this change.

Jasper-Bekkers commented 1 year ago

Could you elaborate on the use-case for this new method? The reason why I'm asking is that currently, the existence of zmq_sys is an implementation detail, and not exposed to the user of the zmq crate, and I am a bit reluctant to change that.

I disagree with this; having abstractions for the sake if it is pretty pointless especially if it gets in the way of performance. If abstractions need to be bypassed this crate should at a minimum facilitate that.