Why is the Connection boxed by default? about quiche HOT 13 CLOSED

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.

from quiche.

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. :)

from quiche.

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.

from quiche.

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?

from quiche.

@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.

from quiche.

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.

from quiche.

@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.

from quiche.

@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.

from quiche.

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?

from quiche.

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

from quiche.

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

from quiche.

Yep, I made that PR :)

from quiche.

Made #249 based on Rust 1.39.

from quiche.