`lightning-net-tokio` does not seem to properly handle TCP closure

[ZmnSCPxj-jr]

Referring to this:

https://blog.netherlabs.nl/articles/2009/01/18/the-ultimate-so_linger-page-or-why-is-my-tcp-not-reliable

The "correct" general pattern, as noted above, is:

• shutdown(fd, SHUT_WR) -- in Tokio terms, use AsyncWriteExt::shutdown
• With a timeout:
  • read() in a loop until you get EOF.
• close() it.

While the BOLT8 protocol DOES provide lengths of expected data, the problem is if the length specifier message AND the subsequent data are both in the same packet scheduled by the kernel --- in that case, both of the length-prefix and the actual message can be disposed by the kernel on close() without being received by the receiving end in case of a clean disconnection request.

This can affect integration tests where we might cause clean disconnection from clean shutdown, which can cause close() in this case that will drop the last message sent to the peer, which now fails the integration test because it was waiting for a message that never comes because of the early close() without the shutdown(). (This really happened over in c-lightning a half-decade ago).

[ZmnSCPxj-jr]

Patch: 0001-lightning-net-tokio-src-lib.rs-Gracefully-shutdown-w.patch

This patch adds libc as a direct dependency. the other option was to add io-util option to tokio, which might cause issues for some library users when Rust unifies features, whereas libc is likely to be very stable.

[TheBlueMatt]

I'm not quite sure I understand why this is a problem, can you clarify what the exact situation you're in is?

If you tell lightning-net-tokio to disconnect it attempts to disconnect immediately, waiting for messages to flush may substantially delay that. Instead, in lightning, we'd rather get the disconnect done so that we can open a new connection (which usually happens almost immediately).

[ZmnSCPxj-jr]

This can affect integration tests where we might cause clean disconnection from clean shutdown, which can cause close() in this case that will drop the last message sent to the peer, which now fails the integration test because it was waiting for a message that never comes because of the early close() without the shutdown(). (This really happened over in c-lightning a half-decade ago).

More specifically, the last message of a mutual channel-close can be lost if you close() the connection after sending closing_signed of the channel (e.g. if it is the last channel with that peer), which will cause the peer to assume you did not properly finish the mutual-channel-closure flow (you think you sent closing_signed and then innocently close()d the TCP connection, but that causes the OS to drop the pending outgoing packet without retrying, meaning the remote peer never receives it) and cause it to unilateral close instead of agreeing to the mutual close.

There may be other cases where the last message sent is important, but because close() potentially drops the last message, it does not get sent, and while a later channel_reestablish should work on LIVE channels, it does not work on CLOSED channels.

[ZmnSCPxj-jr]

An alternative is to use SO_LINGER. But the CORRECT way to use that is to set the socket to blocking and let the close() block which is bad for integrating with async event loops like Tokio.

[TheBlueMatt]

Hmm, is there a reason you want to disconnect immediately after finishing the mutual close? In an integration test you should be able to just wait until both peers are done before you disconnect (as you control both sides). In production, you'll want to stay connected if only because with coop close v3 (soon...) nodes may request a re-sign at any point.

[ZmnSCPxj-jr]

Not in particular. I should note however that as the system becomes more complex, there may be other protocols using BOLT8 tunnels that perform some sort of equivalent of mutual close, where both sides expect to stop talking to each other after their last message, and creating the risk of that last message being dropped when it could have been sent with better assurances seems like an unnecessary increase in risk.

In short, arguably this should be done at the Tokio side, you should ALWAYS close any TCP tunnel using shutdown first, then actually close, because the latter tells the OS that all resources --- including the pending outgoing packet --- can be thrown away. This is one of those "leaky abstraction" things where TCP being actually packet-based underneath leaks out all the way to application layer.

[TheBlueMatt]

I strongly disagree. Nothing using the lightning protocol should be closing the socket immediately (there may be other message handlers that want to talk to that peer!) and generally anything using the lightning protocol should be robust against disconnections, reconnecting and rebroadcasting if needed.