devp2p: Move UDP socket handling from Discovery to Host. #8790

jimpo · 2018-06-05T02:13:08Z

I think moving the UDP IO handling into Host is a better separation, because Host already manages all of the other IO. This gives Discovery a concrete interface in line with how the tests interact with Discovery. Also, it is annoying that Discovery binds to a port in tests, which can cause conflicts.

Even if people decide against this approach, the first commit should get considered, as it is a bug fix.

parity-cla-bot · 2018-06-05T02:13:10Z

It looks like @jimpo signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

tomaka

I think it is indeed better this way.

tomaka · 2018-06-05T07:58:47Z

util/network-devp2p/src/host.rs

+	fn discovery_readable(&self, io: &IoContext<NetworkIoMessage>) {
+		let node_changes = match (self.udp_socket.lock().as_ref(), self.discovery.lock().as_mut()) {
+			(Some(udp_socket), Some(discovery)) => {
+				let mut buf: [u8; MAX_DATAGRAM_SIZE] = unsafe { mem::uninitialized() };


Please initialize this to zero instead of using unsafe.

tomaka · 2018-06-05T08:01:06Z

util/network-devp2p/src/discovery.rs

 	discovery_round: u16,
 	discovery_id: NodeId,
 	discovery_nodes: HashSet<NodeId>,
 	node_buckets: Vec<NodeBucket>,
-	send_queue: VecDeque<Datagramm>,
+	pub send_queue: VecDeque<Datagramm>,


I would be in favour of adding methods to Discovery to pop from the send queue instead of making this field public, but I don't have a strong opinion on this.

jimpo · 2018-06-06T21:22:54Z

@tomaka Thanks for the review, addressed comments.

niklasad1 · 2018-06-07T09:37:50Z

util/network-devp2p/src/discovery.rs

-struct Datagramm {
-	payload: Bytes,
-	address: SocketAddr,
+pub struct Datagramm {


s/Datagramm/Datagram

niklasad1 · 2018-06-07T09:38:31Z

util/network-devp2p/src/discovery.rs

+	}
+
+	pub fn requeue_send(&mut self, datagramm: Datagramm) {
+		self.send_queue.push_front(datagramm)


s/datagramm/datagram

niklasad1 · 2018-06-07T09:38:38Z

util/network-devp2p/src/discovery.rs

-		};
-		event_loop.reregister(&self.udp_socket, Token(self.token), registration, PollOpt::edge()).expect("Error reregistering UDP socket");
-		Ok(())
+	pub fn dequeue_send(&mut self) -> Option<Datagramm> {


s/Datagramm/Datagram

niklasad1 · 2018-06-07T09:40:03Z

util/network-devp2p/src/discovery.rs

@@ -620,8 +566,8 @@ mod tests {
 		let key2 = Random.generate().unwrap();
 		let ep1 = NodeEndpoint { address: SocketAddr::from_str("127.0.0.1:40444").unwrap(), udp_port: 40444 };
 		let ep2 = NodeEndpoint { address: SocketAddr::from_str("127.0.0.1:40445").unwrap(), udp_port: 40445 };
-		let mut discovery1 = Discovery::new(&key1, ep1.address.clone(), ep1.clone(), 0, IpFilter::default());
-		let mut discovery2 = Discovery::new(&key2, ep2.address.clone(), ep2.clone(), 0, IpFilter::default());
+		let mut discovery1 = Discovery::new(&key1, ep1.clone(), IpFilter::default());


ep: NodeEntrypoint is a copy type so no need to explictly call clone

NodeEntrypoint is not Copy, just Clone.

Oh, I got it wrong sorry!

niklasad1 · 2018-06-07T09:41:03Z

util/network-devp2p/src/discovery.rs

@@ -632,14 +578,12 @@ mod tests {
 		discovery2.refresh();

 		for _ in 0 .. 10 {
-			while !discovery1.send_queue.is_empty() {
-				let datagramm = discovery1.send_queue.pop_front().unwrap();
+			while let Some(datagramm) = discovery1.dequeue_send() {


s/datagramm/datagram

niklasad1 · 2018-06-07T09:41:37Z

util/network-devp2p/src/discovery.rs

 				if datagramm.address == ep2.address {
 					discovery2.on_packet(&datagramm.payload, ep1.address.clone()).ok();
 				}
 			}
-			while !discovery2.send_queue.is_empty() {
-				let datagramm = discovery2.send_queue.pop_front().unwrap();
+			while let Some(datagramm) = discovery2.dequeue_send() {


s/datagramm/datagram

niklasad1 · 2018-06-07T09:47:00Z

util/network-devp2p/src/host.rs

 			} else { None }
 		};

 		if let Some(mut discovery) = discovery {
+			let mut udp_addr = local_endpoint.address.clone();


No need for explicit clone for Copy type

niklasad1 · 2018-06-07T09:48:40Z

util/network-devp2p/src/host.rs

+				let writable = discovery.any_sends_queued();
+				let res = match udp_socket.recv_from(&mut buf) {
+					Ok(Some((len, address))) => discovery.on_packet(&buf[0..len], address).unwrap_or_else(|e| {
+						debug!("Error processing UDP packet: {:?}", e);


Please add debug target such as debug!(target: "network", "Error processing UDP packet: {:?}", listen_address);

niklasad1 · 2018-06-07T09:50:00Z

util/network-devp2p/src/host.rs

+					}),
+					Ok(_) => None,
+					Err(e) => {
+						debug!("Error reading UPD socket: {:?}", e);


Please add target, see previous comment!

niklasad1 · 2018-06-07T09:50:20Z

util/network-devp2p/src/host.rs

+				let new_writable = discovery.any_sends_queued();
+				if writable != new_writable {
+					io.update_registration(DISCOVERY)
+						.unwrap_or_else(|e| debug!("Error updating discovery registration: {:?}", e));


Please add target, see previous comment!

niklasad1 · 2018-06-07T09:51:31Z

util/network-devp2p/src/host.rs

+						Ok(Some(size)) if size == data.payload.len() => {
+						},
+						Ok(Some(_)) => {
+							warn!("UDP sent incomplete datagramm");


Please add target, see previous comment!

niklasad1 · 2018-06-07T09:51:45Z

util/network-devp2p/src/host.rs

+							return;
+						}
+						Err(e) => {
+							debug!("UDP send error: {:?}, address: {:?}", e, &data.address);


Please add target, see previous comment!

niklasad1 · 2018-06-07T09:51:54Z

util/network-devp2p/src/host.rs

+					}
+				}
+				io.update_registration(DISCOVERY)
+					.unwrap_or_else(|e| debug!("Error updating discovery registration: {:?}", e));


Please add target, see previous comment!

This works right now because the Host handler happens to be the first one registered on the IoService.

sed -i 's/Datagramm/Datagram/g' util/network-devp2p/src/discovery.rs util/network-devp2p/src/host.rs sed -i 's/datagramm/datagram/g' util/network-devp2p/src/discovery.rs util/network-devp2p/src/host.rs

niklasad1

Thanks, looks good!

* master: Fix subcrate test compile (#8862) network-devp2p: downgrade logging to debug, add target (#8784) Clearing up a comment about the prefix for signing (#8828) Disable parallel verification and skip verifiying already imported txs. (#8834) devp2p: Move UDP socket handling from Discovery to Host. (#8790) Fixed AuthorityRound deadlock on shutdown, closes #8088 (#8803) Specify critical release flag per network (#8821) Fix `deadlock_detection` feature branch compilation (#8824) Use system allocator when profiling memory (#8831) added from and to to Receipt (#8756)

5chdn added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jun 5, 2018

5chdn added this to the 1.12 milestone Jun 5, 2018

tomaka suggested changes Jun 5, 2018

View reviewed changes

tomaka added the A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. label Jun 5, 2018

debris removed the A0-pleasereview 🤓 Pull request needs code review. label Jun 5, 2018

jimpo force-pushed the discovery-io branch from acc5a80 to 2a205f4 Compare June 6, 2018 19:23

5chdn added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jun 7, 2018

tomaka approved these changes Jun 7, 2018

View reviewed changes

tomaka added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 7, 2018

niklasad1 reviewed Jun 7, 2018

View reviewed changes

rphmeier added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Jun 7, 2018

devp2p: Fix bug with potentially incorrect UDP registration.

1e6fe51

This works right now because the Host handler happens to be the first one registered on the IoService.

jimpo added 5 commits June 7, 2018 11:10

devp2p: Move UDP socket handling from Discovery to Host.

af75814

devp2p: Use 0-initialized memory buffer instead of unsafe.

46fe32b

Remove send_queue field from public interface of Discovery.

e9c983e

Rename Datagramm to Datagram.

d469480

sed -i 's/Datagramm/Datagram/g' util/network-devp2p/src/discovery.rs util/network-devp2p/src/host.rs sed -i 's/datagramm/datagram/g' util/network-devp2p/src/discovery.rs util/network-devp2p/src/host.rs

Include target in log statements.

deb5253

jimpo force-pushed the discovery-io branch from 2a205f4 to deb5253 Compare June 7, 2018 18:10

niklasad1 approved these changes Jun 7, 2018

View reviewed changes

niklasad1 added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jun 7, 2018

5chdn added the B0-patchthis label Jun 8, 2018

5chdn merged commit 13bc922 into openethereum:master Jun 8, 2018

jimpo deleted the discovery-io branch June 13, 2018 16:23

5chdn mentioned this pull request Jun 18, 2018

Release next-beta 1.11 #8190

Closed

57 tasks

andresilva mentioned this pull request Jun 18, 2018

[beta] Backports #8916

Merged

18 tasks

5chdn removed the B0-patchthis label Jun 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

devp2p: Move UDP socket handling from Discovery to Host. #8790

devp2p: Move UDP socket handling from Discovery to Host. #8790

jimpo commented Jun 5, 2018

parity-cla-bot commented Jun 5, 2018

tomaka left a comment

tomaka Jun 5, 2018

tomaka Jun 5, 2018

jimpo commented Jun 6, 2018

niklasad1 Jun 7, 2018

niklasad1 Jun 7, 2018

niklasad1 Jun 7, 2018

niklasad1 Jun 7, 2018

jimpo Jun 7, 2018

niklasad1 Jun 7, 2018

niklasad1 Jun 7, 2018

niklasad1 Jun 7, 2018

niklasad1 Jun 7, 2018

niklasad1 Jun 7, 2018

niklasad1 Jun 7, 2018

niklasad1 Jun 7, 2018

niklasad1 Jun 7, 2018

niklasad1 Jun 7, 2018

niklasad1 Jun 7, 2018

niklasad1 left a comment

devp2p: Move UDP socket handling from Discovery to Host. #8790

devp2p: Move UDP socket handling from Discovery to Host. #8790

Conversation

jimpo commented Jun 5, 2018

parity-cla-bot commented Jun 5, 2018

tomaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimpo commented Jun 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 left a comment

Choose a reason for hiding this comment