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

Use smart pointers in SocketSet's API #42

Closed
wants to merge 1 commit into from

Conversation

batonius
Copy link
Contributor

Problem: Currently SocketSet return sockets as &mut Socket which makes impossible to automatically track state changes, which, in turn, makes implementing packet dispatch problematic.

Solution: Implement SocketRef smart reference, which automatically calls bookkeeping code at the end of a usage session. Change SocketSet to return SocketRef instead of &mut Socket.

Changes introduced by this pull request:

  • the SocketRef struct has been added;
  • the SocketSession trait for socket type-specific usage session cleanup has been added;
  • SocketSet returns SocketRef instead of &mut Socket;
  • the AsSockettrait has been refactored into FromSocket;

Other: Currently SocketRef holds only socket's handle and &mut *Socket, and all SocketSession::finish implementations are empty.

This PR is a part of #19 effort.

pub(crate) fn from_socket_ref(mut socket_ref: SocketRef<'a, Socket<'b, 'c>>)
-> Option<SocketRef<'a, T>> {
if let Some(socket) = T::from_socket(socket_ref.socket) {
socket_ref.fused = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible I'm missing something here: I want to prevent a spurious destructor call, but I can't use mem::forget since socket_ref is being borrowed from until the end of the method. It doesn't prevent the destructor from being called tho, so I have to do it by hand.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option<&mut T> maybe? If I understand correctly, this Option would be deconstructed exactly three times, one in constructor, one in destructor and one in deref, so the one added jump is basically nothing. Then just take the option.

Not worth introducing unsafe code just for this IMO.

Copy link
Contributor Author

@batonius batonius Sep 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option<&mut T> in SocketRef would mean DerefMut::deref_mut could panic, and there's really no reason for it. I guess I leave it as it is for now.

Copy link
Contributor

@whitequark whitequark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I'm significantly happier about this PR than the code you've submitted before.

@@ -49,14 +48,14 @@ fn main() {
let tcp_handle = sockets.add(tcp_socket);

{
let socket: &mut TcpSocket = sockets.get_mut(tcp_handle).as_socket();
let mut socket = sockets.get::<TcpSocket>(tcp_handle).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe dispatching over the socket type is the rare case (it only happens inside smoltcp and in OS/firmware code that's generic over the socket, e.g. fd handling), so let's have a panicking .get() and a non-panicking get_opt(), seems cleaner than unwrapping everywhere.

let mut device_result = Ok(());
let socket_result =
match socket {
&mut Socket::Raw(ref mut socket) =>
match *(&mut *socket) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

squint what... is this thing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like I've got carried away a little.

@@ -16,6 +16,7 @@ mod raw;
mod udp;
mod tcp;
mod set;
mod socket_ref;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mod ref, use self::ref::Ref and then reexport as a prefixed name; that's how everything else is done here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ref is a keyword, so I can't use mod ref;, I'll rename socket_ref::SocketRef to socket_ref::Ref.

}
}

impl<'a, T: SocketSession> Deref for SocketRef<'a, T> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need Deref for SocketRef since all useful methods on sockets use &mut self anyway. It seems like this would lead to people thinking they can do something with &XxxSocket.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DerefMut requires Deref.

impl<'a, 'b> SocketSession for UdpSocket<'a, 'b> {}
impl<'a> SocketSession for TcpSocket<'a> {}

impl<'a, 'b> SocketSession for Socket<'a, 'b> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't (and can't, Rust enums are not extensible) support new socket types defined in external crates, so the entire SocketSession logic can simply be folded into the SocketRef Drop impl.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SocketRef is generic over socket type, we can't put socket-specific cleanup code there. Besides, I'm going to extend SocketSession with an associated type that holds socket's state, something like https://github.com/batonius/smoltcp/blob/packet_dispatch/src/socket/tracker.rs#L11 .

pub fn iter_mut<'d>(&'d mut self) -> IterMut<'d, 'b, 'c> {
IterMut { lower: self.sockets.iter_mut() }
IterMut { lower: self.sockets.iter_mut(), index: 0 }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be more elegant to use .iter_mut().enumerate() here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I did initially, but std::iter::Enumerate requires std, so I have to do it manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently, I've missed core::iter.

/// which is rather inelegant. Conversely, when `dispatch` is called, the packet length is
/// not yet known and the packet storage has to be allocated; but the `&PacketRepr` is sufficient
/// since the lower layers treat the packet as an opaque octet sequence.
/// the [FromSocket](trait.FromSocket.html) trait, and call e.g. `UdpSocket::from_socket(socket)`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth mentioning SocketSet here and that they're probably looking for socket_set.get::<TcpSocket>(tcp_handle) instead

fn as_socket(&mut self) -> &mut T;
fn try_as_socket(&mut self) -> Option<&mut T>;
pub trait FromSocket<'a, 'b>: SocketSession + Sized {
fn from_socket<'c>(&'c mut Socket<'a, 'b>) -> Option<&'c mut Self>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

downcast_socket maybe? from_socket is not really indicative of what this does.

pub(crate) fn from_socket_ref(mut socket_ref: SocketRef<'a, Socket<'b, 'c>>)
-> Option<SocketRef<'a, T>> {
if let Some(socket) = T::from_socket(socket_ref.socket) {
socket_ref.fused = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option<&mut T> maybe? If I understand correctly, this Option would be deconstructed exactly three times, one in constructor, one in destructor and one in deref, so the one added jump is basically nothing. Then just take the option.

Not worth introducing unsafe code just for this IMO.

@batonius
Copy link
Contributor Author

batonius commented Oct 3, 2017

Updated.

@whitequark
Copy link
Contributor

whitequark commented Oct 5, 2017

Merged manually with... lots of changes.

  • Some renames.
  • A lot of doc improvement.
  • Merged FromSocket::downcast and ::from_socket_ref functions into one, using Ref::map (like libcore's own BorrowMut).
  • Folded SocketHandle into sockets themselves since they basically already have that in form of debug_id.
  • Removed try_get since it's not used anywhere.
  • Made panics in SocketSet more informative.

Please study them and take note.

Sorry, something went wrong.

@whitequark whitequark closed this Oct 5, 2017
@batonius batonius deleted the socket_ref branch October 5, 2017 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants