Polling networking#179
Conversation
This allows a thread to more easily wait for packets on multiple sockets at the same time.
The new approach allows polled sending, as well as separating off send timestamps. Unfortunately, timestamp reception still requires a future to be created, as it is currently impossible to poll for error queue readyness.
davidv1992
force-pushed
the
polling-networking
branch
from
90b8eaa to
7f7ddf0
Compare
This allows it to be kept around in datastructures for emulating poll functionality, without running into self-referential struct problems. This is needed as tokio's AsyncFd doesn't support poll for the Error interest, only Read and Write.
This adds proper dealing with the padding fields for libc::cmsghdr.
davidv1992
force-pushed
the
polling-networking
branch
3 times, most recently
from
6954db2 to
83c205b
Compare
This also updates the testing to do more thorough testing accross platforms, as that is creating just too many issues.
davidv1992
force-pushed
the
polling-networking
branch
from
83c205b to
f126e41
Compare
rnijveld
left a comment
rnijveld
left a comment
There was a problem hiding this comment.
A few small things, but looks good overall!
| } | ||
| } | ||
|
|
||
| pub(super) fn fetch_send_timestamp_try_read( |
There was a problem hiding this comment.
I don't think this needs to be pub(super) anymore? Could also just be a standalone function instead of an associated one.
There was a problem hiding this comment.
Done, keeping the caller associated even though strictly that isn't necessary, as it makes calling it easier.
| } | ||
|
|
||
| Ok(send_ts) | ||
| Ok(counter.zip(send_ts)) |
There was a problem hiding this comment.
Could it happen that the msg_errqueue contains a message that omits the timestamp control message entirely? I.e. should this function not return Result<Option<(u32, Option<FullTimestampData>)>> (or default back to a FullTimestampData that has two None values in case it is missing)?
There was a problem hiding this comment.
So I think this should never happen in a way that should make us potentially ignore future availability of the send timestamps. The example scenario is when the remote responds with an ICMP message to the sent package, which then arrives before the timestamp. The packet was still sent so the timestamp is coming, but there can occur an error message from the error queue beforehand telling us about the ICMP response.
| } | ||
| } | ||
|
|
||
| // This potentially drops multiple timestamps. Therefore, it isn't really supposed to be used |
There was a problem hiding this comment.
I think we can make this a doc comment and expand a little on what conditions are required when using this function.
There was a problem hiding this comment.
done, also added comments on the relevant call sites.
Occured in response to review commments from rnijveld.
davidv1992
force-pushed
the
polling-networking
branch
from
7dbc5a5 to
099e4c7
Compare
2 participants
This implements changes to allow polling networking, with only shared borrows on the socket.
It also contains a number of bugfixes for compilation on MacOS, as well as changes to the CI to more proactively test that.
Finally, it smoothes over a few minor API wrinkles not directly related to polling networking.