network: Detect early that NotificationOutSubstream was closed by the remote

[dmitry-markin]

Detect that NotificationOutSubstream was closed by the remote even if we don't write data into it.
Fixes paritytech/polkadot-sdk#526.

Description

In Notifications we use two duplex substreams for outgoing and incoming notifications. After the initial handshake, each substream becames a unidirectional stream of data. Unfortunately, Sink::poll_flush called on libp2p socket does not produce an error if the substream was dropped on the remote until we write some data into it. In order to overcome this limitation, this PR adds polling of the out_substream for incoming messages (which should be none) in <NotificationOutSubstream as Sink>::poll_flush to detect that the stream has terminated (was dropped/closed by the remote).

It may be worth filing a bug to libp2p to expose that the stream was dropped (reset) by the remote via Sink::poll_flush even if no data was written into it before the flushing.

[melekes]

👍

[altonen]

Test case showing that it detects the closed substream correctly without writing to it would be nice

[bkchr]

It may be worth filing a bug to libp2p to expose that the stream was dropped (reset) by the remote via Sink::poll_flush even if no data was written into it before the flushing.

Sounds like a good idea!

[dmitry-markin]

Test case showing that it detects the closed substream correctly without writing to it would be nice

Done! As a bonus we got a test for poll_process() :)

[dmitry-markin]

It may be worth filing a bug to libp2p to expose that the stream was dropped (reset) by the remote via Sink::poll_flush even if no data was written into it before the flushing.

Sounds like a good idea!

Actually, I observed the same behavior on a raw tokio socket, so probably libp2p just mimics sockets with its substreams.

[dmitry-markin]

It may be worth filing a bug to libp2p to expose that the stream was dropped (reset) by the remote via Sink::poll_flush even if no data was written into it before the flushing.

Sounds like a good idea!

Actually, I observed the same behavior on a raw tokio socket, so probably libp2p just mimics sockets with its substreams.

To be honest, I'm not quite sure now that extending poll_flush() API to detect the stream termination was the best idea — may be we should add a different method poll_terminated() and call it on all substreams from Notifications?

CC @altonen

[altonen]

When and where would you call poll_terminated()? I'm in favor of having this functionality either way but I feel as though having a separate call for just polling termination is not the right way to go, unless I missed something. Having said that, there's an underlying deeper problem both with the handshakes and block request handling that we need to address ASAP, which hopefully will alleviate these problems we have with peering mismatches and syncing.

[dmitry-markin]

When and where would you call poll_terminated()? I'm in favor of having this functionality either way but I feel as though having a separate call for just polling termination is not the right way to go, unless I missed something. Having said that, there's an underlying deeper problem both with the handshakes and block request handling that we need to address ASAP, which hopefully will alleviate these problems we have with peering mismatches and syncing.

Okay, I will just add a comment to Notifications that poll_flush in NotificationsOutSubstream carries information about substream termination, so we need to call poll_flush on all substreams, and not only on those we have written some data to (this is already the case).