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

Problem with Endpoint lifetime #81

Open
blabaere opened this issue Dec 22, 2014 · 8 comments
Open

Problem with Endpoint lifetime #81

blabaere opened this issue Dec 22, 2014 · 8 comments

Comments

@blabaere
Copy link
Collaborator

The rust nightly build cc19e3380 2014-12-20 introduced the warning reproduced below.
This drew my attention to the fact this conflicting lifetime name wasn't even used in the function !
So after resolving the name conflict, I changed the signature parameter to actually use that named lifetime but this had the effect of breaking almost all unit tests !!! 💥

With this modification, it becomes impossible to retrieve the endpoint and then to send or receive a message. For example you can use test_bind alone, but not test_bind and then test_write.
Calling directly bind without storing the result in variable and then write works fine.
Storing the NanoResult or the Endpoint will cause a compilation error.

cannot borrow push_socket as mutable more than once at a time.

It looks like constraining the lifetime of the Endpoint to the one of the Socket makes the Endpoint a 'borrower' of the Socket.

I will post something on reddit and add the link here when done.

Signature before and after the change:
fn bind<'b, 'a: 'b>(&mut self, addr: &str) -> NanoResult<Endpoint<'b>>
fn bind<'b, 'x: 'b>(&'x mut self, addr: &str) -> NanoResult<Endpoint<'b>>
Warning:
   Compiling nanomsg v0.3.0 (file:///home/travis/build/thehydroimpulse/nanomsg.rs)
/home/travis/build/thehydroimpulse/nanomsg.rs/src/lib.rs:203:21: 203:23 warning: lifetime name `'a` shadows another lifetime name that is already in scope
/home/travis/build/thehydroimpulse/nanomsg.rs/src/lib.rs:203     pub fn bind<'b, 'a: 'b>(&mut self, addr: &str) -> NanoResult<Endpoint<'b>> {
                                                                                 ^~
/home/travis/build/thehydroimpulse/nanomsg.rs/src/lib.rs:123:6: 123:8 help: shadowed lifetime `'a` declared here
/home/travis/build/thehydroimpulse/nanomsg.rs/src/lib.rs:123 impl<'a> Socket<'a> {
                                                                  ^~
/home/travis/build/thehydroimpulse/nanomsg.rs/src/lib.rs:203:21: 203:23 help: shadowed lifetimes are deprecated and will become a hard error before 1.0
@thehydroimpulse
Copy link
Owner

Yeah, I recently hit this issue myself. There's a bug in the rustc compiler that prevents using lifetime constraints and using the same lifetime against &self. rust-lang/rust#18232

It appears like the issue has been fixed already so we can start using it.

Edit: ahh, it seems like you've already tried it.

@thehydroimpulse
Copy link
Owner

Your example of

fn bind<'b, 'x: 'b>(&'x mut self, addr: &str) -> NanoResult<Endpoint<'b>>

Is what I imagined to work, but that doesn't seem to be the case.

@blabaere
Copy link
Collaborator Author

It works when it comes to prevent someone from using an endpoint outside the scope of a socket.
The following scenario is correctly reported as an error by the compiler.

let mut e;
{
    let mut socket = ...
    e = socket.bind(...).unwrap();
}
e.shutdown();

But we have now a problem because it seems that the socket mutability has been transfered to the endpoint.

@blabaere
Copy link
Collaborator Author

The suggestions I got on irc are

  • Clone and Copy : we don't want this.
  • Move the read and write functions to the endpoint : big change, not sure if feasible
  • Have the bind/connect functions return a socket and an endpoint : feel strange but could work.

But we could also completely remove the lifetime constraint until we find the solution.

@thehydroimpulse
Copy link
Owner

Yeah, I think option 1 and 2 are a no go. Option 3 seems a bit contrary to the goal of having M endpoints to N sockets.

I think the best option would be to remove the constraints until we find another solution like you suggested.

@blabaere
Copy link
Collaborator Author

Another option would be to make the self parameter of bind and connect const instead of mut.
But that does feel like lying.

@thehydroimpulse
Copy link
Owner

@blabaere I'd actually be ok with that because it would be considered interior mutability. As long as we can prove it to be safe given the extra freedom you have with &self versus &mut self, it would be ok.

Trying to think of what problems could occur if we were to do that, but none are coming up atm.

@blabaere
Copy link
Collaborator Author

I spoke too fast, that works only when not keeping the endpoint ...

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