search

status-im / nim-chronos

Chronos - An efficient library for asynchronous programming
https://status-im.github.io/nim-chronos/docs/chronos
Apache License 2.0
353 stars 51 forks source link

TCP write cancellation support #353

Closed Menduist closed 1 year ago

Menduist commented 1 year ago

This PR allows to cancel TCP write, if they are not already begun Seems to work on every platform, surprisingly

cheatfate commented 1 year ago
  1. What the reason to cancel write request?
  2. Additional Future[T] for every write request is pretty heavy price for it.
  3. Why PIPE test is missing? Maybe its not working?
Menduist commented 1 year ago

What the reason to cancel write request?

The main use-case right now is the upcoming "big blocks" for ethereum. While sending a big block to a peer (which can take a long time on slow connections), we may realize that this was actually useless since the peer already know the block

If we have the opportunity to cancel it (if it's still inside the queue), that would save bandwidth

Additional Future[T] for every write request is pretty heavy price for it.

I agree, do you think we should change the Future system to let "callback futures" to swallow cancel requests, or just not swallow the cancel?

The nice thing about swallowing is that the caller can know if the data got sent or not, which is for instance used in the test to know how much data to expect

Why PIPE test is missing? Maybe its not working?

I didn't think about PIPE, I'll add tests

cheatfate commented 1 year ago

What the reason to cancel write request?

The main use-case right now is the upcoming "big blocks" for ethereum. While sending a big block to a peer (which can take a long time on slow connections), we may realize that this was actually useless since the peer already know the block

This is totally insane, just because you talking about high application level in this model. For example

  1. nimbus sends "big block" using libp2p stream
  2. libp2p stream uses libp2p muxer to insert this stream into libp2 connection (And there a lot of other libp2p streams could reuse this single libp2p connection).
  3. libp2p connection provides encryption and keeps its own counters/states and writes to libp2p transport.
  4. libp2p transport uses chronos transport to write data to TCP socket.

And you provide cancellation support to drop packets which should be sent to remote side... So at any moment if you drop some of the packets written you already break 3, 2 and who knows what will happens with 1. Also because 2 would be definitely broken (some of the data removed from the chronos writing queue could be related to other streams), it become impossible to recover and reset libp2p muxer. So the only way for you to recover the connection is to drop TCP connection and establish a new one.

Menduist commented 1 year ago

You are right, I forgot that the encryption layer was stateful and thus we couldn't just remove messages from the queue, without having to re-encrypt the remaining queue

So any cancellation should happen at the muxer level, not in chronos. I'll close this PR since I don't need it anymore