Genymobile / gnirehtet

Gnirehtet provides reverse tethering for Android
Apache License 2.0
6.23k stars 571 forks source link

Notes for changing to edge-triggers in Mio #208

Open Thomasdezeeuw opened 5 years ago

Thomasdezeeuw commented 5 years ago

Following https://github.com/tokio-rs/mio/issues/928.

Generally speaking when changing from level to edge trigger you need to put read/write in a loop, because it could that after a call to read/write another one is ready once the first one is done.

TcpConnection needs to basically call read in a loop here: https://github.com/Genymobile/gnirehtet/blob/2bf8a3cfbe2168031897b8f917c088fe6d47710c/relay-rust/src/relay/tcp_connection.rs#L319

And the same goes for UdpConnection here: https://github.com/Genymobile/gnirehtet/blob/2bf8a3cfbe2168031897b8f917c088fe6d47710c/relay-rust/src/relay/udp_connection.rs#L170

I think if you put those two reads in a loop you can change both of them to edge triggers.

/cc @rom1v

rom1v commented 5 years ago

@Thomasdezeeuw Thank you for your review.

Generally speaking when changing from level to edge trigger you need to put read/write in a loop, because it could that after a call to read/write another one is ready once the first one is done.

After the first read is done (and the data is handled), I may want to not read on purpose (and reregister without a "read interest"), typically because I may not process the data now (e.g. if the socket I want to forward to is not available for writing):

https://github.com/Genymobile/gnirehtet/blob/2bf8a3cfbe2168031897b8f917c088fe6d47710c/relay-rust/src/relay/tcp_connection.rs#L728-L750

With edge-triggered notifications, I think I either need to manage extra buffering (to read the whole available data on edge notification), or to add additional flags to "simulate" level-triggered notifications.

Thomasdezeeuw commented 5 years ago

If you don't want to read anymore you'll likely need to re-register with write only interest (or at least without read interests) and at the point you want to read again re-register with read interest, then you should get events for reading again even if data was available prior to that.

rom1v commented 5 years ago

Thanks, I'll try that :+1: