Why is the Connection boxed by default?

[NeoLegends]

Hey,

I was just skimming over the API and I was wondering why connect and accept return a boxed Connection? Connection is neither a trait object nor dynamically sized, so why the need for the box?

[ghedo]

Hello,

Connection is self-referencing, so it can't be moved or the internal reference (which is actually a raw pointer) would become invalid. So we use Box to get a more stable address on the heap. In theory we should use Pin<Box<...>> but Pin doesn't expose the APIs we need.

[NeoLegends]

Ahh, thanks for the clarification. One more question: could this be hidden inside Connection‘s API?

This feels like a nitty gritty API internal to me that shouldn‘t be exposed to the user (at least from my limited understanding). Especially because the user might be tempted to move the connection out of the box, effectively breaking it. For example, Connection could be made into a newtype around the current Connection.

And if that‘s not possible, I think it should at least be documented that the boxing is intentional. :)

[ghedo]

I'm not sure the indirection of wrapping Connection would be worth the effort, though I do agree that this should be documented. I'll reopen the issue to track that part.

[NeoLegends]

I could file a PR to do this. I'd just need some guidance as to what to do with the documentation on Connections methods. Should they move to the newtype or be copied (so that they'd exist twice)? And also, would you be fine with moving the connection implementation from lib.rs into a separate module, so that lib.rs solely wraps Connection into a newtype and exports the public API surface?

[ghedo]

@NeoLegends it's not only about actually having to do the initial work to implement that, but the maintainance burden the change adds in the future (e.g. the public Connection APIs would need to be duplicated to handle the wrapped, which makes it somewhat harder to understand and add new APIs).

So for the moment I'd rather not do anything more than documenting the current situation. We can always reconsider in the future.

[RReverser]

e.g. the public Connection APIs would need to be duplicated to handle the wrapped

I don't think this is true - you already accept it by reference, and it will be perfectly fine to pass pointer from Box as if it was a pointer to unwrapped structure.

[ghedo]

@RReverser if I understand correctly, the proposal here is to have something like pub struct ConnectionWrapper(Box<Connection>) and make Connection itself private, so you'd have to mirror the private Connection API in ConnectionWrapper.

[NeoLegends]

@ghedo Correct. I think though, that the additional maintenance overhead will be minimal and that the safety gains outweigh this. When I made my first tests (and bumped into the questions I asked), the newtype added a mere 100 lines or so to lib.rs, not including some one-time churn like converting connect / accept to return the newtype and so on.

[cramertj]

Connection is self-referencing, so it can't be moved or the internal reference (which is actually a raw pointer) would become invalid. So we use Box to get a more stable address on the heap. In theory we should use Pin<Box<...>> but Pin doesn't expose the APIs we need.

FWIW there's nothing stopping users from pulling the connection out of the box and putting it back in another one, triggering UB. What specific APIs do y'all need that Pin<Box<Connection>> wouldn't allow?

[ghedo]

@cramertj basically this rust-lang/rust#60245, though it could potentially be implemented in some other uglier way.

[NeoLegends]

@ghedo i believe the required features will be stabilized as part of 1.39. rust-lang/rust#63985

[ghedo]

Yep, I made that PR :)