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

Structs from runng do not implement Debug and might benefit from some other nice Traits. #25

Closed
najamelan opened this issue Jan 24, 2019 · 4 comments

Comments

@najamelan
Copy link
Contributor

najamelan commented Jan 24, 2019

Right now if a struct contains for example a protocol::pair1::Pair1 it can't derive Debug because Pair1 doesn't implement it. It is generally accepted to more or less always derive or implement at least Debug for all pub structs.

Others that might be useful can be found in the rust book. Eq might also make sense for a socket. For the difference between Eq and PartialEq, see here.

Some traits are handy if it makes sense, like Copy, but they have implications for the future. If you later add a non-Copy property to your struct, you will have to break the API by removing Copy and instances will be implicitly copied on the stack which might not always be a good idea. However Clone is explicit and might be a good idea.

@najamelan
Copy link
Contributor Author

I forgot to mention. Sometimes you have to explicitly set structs to be not Send or Sync if they are not threadsafe. Especially when working with C code it's good to think about this for every structure.

@jeikabu
Copy link
Owner

jeikabu commented Jan 25, 2019

This one is trickier.

I've been using #derive[] sparingly on an "as-needed" basis. For example, NngError has Clone/Copy/Debug/PartialEq.

Most of the other structs are wrappers around C FFI bindings and not safe for Copy/Clone. Pretty much all of them have a function that needs to be called on Drop (nng_msg_free, nng_close, etc.). As much as possible I'd like to use Rust to enforce "correct" usage, but I haven't really settled on the best way to deal with this:

For example, once you've called create_async_context() I don't think it's safe to use the socket (because nng_aio will be using it), so the socket should probably get "consumed" (i.e. create_async_context(self) instead of create_async_context(&self)). But I need to ask the nng author to be sure.
Pipes complicate this because they can be obtained from a message, but closing them will make the associated nng_ctx/nng_aio fail...

But I think you're right that there could be some improvements:

@najamelan
Copy link
Contributor Author

Thanks for looking into this. This makes me wonder, in async code it is common to use closures and thus often one needs to "clone" things. In the little test app I posted, I thus end up cloning a protocol::pair1::Pair1 even though it's not in polyamorous mode. Is that a problem?

@jeikabu
Copy link
Owner

jeikabu commented Jan 25, 2019

It's ok to clone it, it won't get closed until the last reference goes away. The Arc provides Send/Sync.

At the moment there's only impl Send for NngAio because I tend to write:

let a = factory.pair_open()?.listen(&url)?;
let b = factory.pair_open()?.dial(&url)?;

let a_thread = thread::spawn(move || -> NngReturn {
    let mut ctx = a.create_async_context()?;
    //...
});

I don't plan to use the synchronous functions like SendMsg::send() so I've not thought about that part of the API much. If you do plan to use that part, feel free to make suggestions or a PR. Come to think of it, I'm not even sure its got tests.

jeikabu added a commit that referenced this issue Feb 12, 2019
* Numerous improvements to Result and error-handling

- Rename NngResult to more canonical `Result` and get rid of NngReturn
- Rename NngFail to more canonical runng::Error
	- TryFrom conversion for NngFail
	- impl std::error::Error (fix #24)
	- Err and Unknown enums now Errno and UnknownErrno, respectively.
- Rename `NngFail::succeed_then()` to `zero_map()`
	- Zero is `Ok` and then it's the same as `Result::map`
- Tests use fewer glob (*) imports
- impl AsRef/AsMut<[u8]> for Alloc and NngMsg (#25)
	- https://rust-lang-nursery.github.io/api-guidelines/interoperability.html#conversions-use-the-standard-traits-from-asref-asmut-c-conv-traits
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

3 participants