Bootstrap/use read loop #3885
Bootstrap/use read loop #3885
Conversation
Ben-PH
commented
Apr 28, 2023
•
Ben-PH
commented
- document all added functions
- try in sandbox /simulation/labnet
- unit tests on the added/changed features
- make tests compile
- make tests pass
- add logs allowing easy debugging in case the changes caused problems
- if the API has changed, update the API specification
There was a problem hiding this comment.
In reference to #3842 (comment)
also sends message from server in a single message, making better use of the channel. This was done in this PR as the inefficiency was revealed while refining the peek helper method.
| pub(crate) use client::*; | ||
| pub(crate) use server::*; | ||
|
|
||
| trait BindingReadExact: io::Read { |
There was a problem hiding this comment.
for a side-by-side view of this diff, you can exclude the fmt and module re-arrangement commit.
| // read the known-len component of the message | ||
| let known_len = SIGNATURE_SIZE_BYTES + self.size_field_len; | ||
| let mut known_len_buff = vec![0u8; known_len]; | ||
| // TODO: handle a partial read | ||
| self.read_exact_timeout(&mut known_len_buff, deadline) | ||
| .map_err(|(err, _consumed)| err)?; | ||
|
|
||
| // construct the signature | ||
| let sig_array = known_len_buff.as_slice()[0..SIGNATURE_SIZE_BYTES] |
There was a problem hiding this comment.
see conversation here: #3842 for motivation behind reverting the use of peek. Original intent was to make the very notion of needing to handle a partial read, as far as the stream state is concerned, as obsolete. If this approach could be valuable, using it now is very premature
| // send signature | ||
| self.duplex.set_write_timeout(duration)?; | ||
| self.duplex.write_all(&sig.to_bytes())?; | ||
| // construct msg length, and convert to bytes | ||
| let msg_len_bytes = msg_len.to_be_bytes_min(self.max_bootstrap_message_size)?; | ||
|
|
||
| // send message length | ||
| { | ||
| let msg_len_bytes = msg_len.to_be_bytes_min(self.max_bootstrap_message_size)?; | ||
| self.duplex.write_all(&msg_len_bytes)?; | ||
| } | ||
| // organize the bytes into a sendable array | ||
| let stream_data = [sig.to_bytes().as_slice(), &msg_len_bytes, &msg_bytes].concat(); | ||
|
|
||
| // send message | ||
| self.duplex.write_all(&msg_bytes)?; | ||
| // send the data | ||
| self.duplex.set_write_timeout(duration)?; | ||
| self.duplex.write_all(&stream_data)?; | ||
|
|
||
| // save prev sig | ||
| // update prev sig | ||
| self.prev_message = Some(Hash::compute_from(&sig.to_bytes())); |
There was a problem hiding this comment.
Should make for more efficient use of the stream, at the expense of slight latency increase, as the first byte gets sent a touch later.
However, by collecting all data into a single buffer to be sent, it simplifies the code, and makes testing easier (test_bootstrap_server was failing on timeout, and this change fixed that).
|
Request re-review as many changes were made in response to #3885 |
