KolbyML
commented
11 months ago CI is failing because of this test close_succeeds_if_only_fin_ack_dropped
let mut recv_stream = recv_stream_handle.await.unwrap();
match timeout(EXPECTED_IDLE_TIMEOUT * 2, recv_stream.close()).await {
Ok(Ok(_)) => {} // The receive stream should close successfully: only FIN-ACK is missing
Ok(Err(e)) => panic!("Error closing receive stream: {:?}", e),
Err(e) => {
panic!("The recv stream did not timeout on close() fast enough, giving up after: {e:?}")
}
};on line Ok(Err(e)) => panic!("Error closing receive stream: {:?}", e), with Error closing receive stream: Kind(NotConnected)
The issue is because the connection is already closed because we do one way fins now. So when we try to do .close() we get this error cause we are trying to close a closed stream. So I assume we remove the close check on recv_stream @carver thoughts?
carver
Why
Every other uTP implementation uses one way fins.
We on the other hand use two way fins, like TCP. This has some advantages in some cases. But every usecase of uTP has been sending data one way.
Because we use two way and fluffy uses one way we have this problem where we only try to handle one way fins on the timeout of our connection which delays us finishing the uTP connection currently on Trin this is set to 1.25 seconds, this delay might also grow in size but that symantic interaction doesn't matter and any delay is enough justification to make this change. This delay will happen whenever we interact with Fluffy or Ultralight and if we want adoption this is unacceptable.
This is also currently causing interop issues with our bridge, well we can add another timeout exception for this case, it wouldn't fix the 1.25s delay so we might as well just implement 1 way fins.
https://portal-hive.ethdevops.io/?page=v-pills-results-tab&suite=1701568288-22f1ef76b380011a7f14fa3c85205088.json Here is an example where these tests are failing because Trin thinks the uTP transfer is timing out, fluffy thinks it is successful. And in reality fluffy got the data, and we are waiting for no reason. We also spam fluffy with 8 fin packets for no reason which is another reason to do 1 way fins.
I tested this change on portal-hive and all tests passed using it using Trin and Fluffy. I tested the trin-bridge test again between fluffy and trin and it worked so it definitly fixes the issue I was seeing. In this process I also double checked the uTP logs to make sure there wasn't anything funny.
Here are the results with the change, the link above shows it all failing.
what did I do
on_packet()for the sender and receiver cases for handling one way ackswrite-bufferwas written. The while pending_read.pop_front statements doesn't work, because it relied on a gridlock system so it would be impossible for it to even loop more then once. Because the loop in stream.rs only added a new one shot channel, when it was done handling the last one. So the while loops was functionally useless inprocess_reads. I also needed to do it this way so we would be able to flush the recv-buffer before closing the connection.