[request-response] Allow for a way to close the substream

[thomaseizinger]

Closing the substream in a request-response protocol is a convenient way of signaling an error to the other peer.

Unfortunately, the only way I've been able to achieve that at the moment is by creating a response enum:

https://github.com/comit-network/comit-rs/blob/2cca5fef69b49337279c16f4f7421b8052e4cbd0/comit/src/network/protocols/announce.rs#L352-L374

This has the downside that the sender side has to deal with the enum as well although the response never makes it over the wire.

https://github.com/comit-network/comit-rs/blob/2cca5fef69b49337279c16f4f7421b8052e4cbd0/comit/src/network/protocols/announce.rs#L205-L217

Would it be possible to add an interface to RequestResponse that allows to close a substream? Maybe dropping the ResponseChannel could do that automatically?

[romanb]

Maybe dropping the ResponseChannel could do that automatically?

If you drop the ResponseChannel, then the InboundUpgrade should notice and finish, resulting in the substream getting closed. This is what allows one-way protocols that don't have responses. Does that not work for you?

[thomaseizinger]

Maybe dropping the ResponseChannel could do that automatically?

If you drop the ResponseChannel, then the InboundUpgrade should notice and finish, resulting in the substream getting closed. This is what allows one-way protocols that don't have responses. Does that not work for you?

I think what I would want is:

fn upgrade_inbound(mut self, mut io: NegotiatedSubstream, protocol: Self::Info) -> Self::Future {
    async move {
        let read = self.codec.read_request(&protocol, &mut io);
        let request = read.await?;
        if let Ok(()) = self.request_sender.send(request) {
            match self.response_receiver.await {
				Ok(response) => {
					let write = self.codec.write_response(&protocol, &mut io, response);
                    write.await?;
				}
				Err(_) => {
					io.close().await;
				}
            }
        }
        Ok(())
    }.boxed()
}

If the substream is not explicitely closed, the side waiting for the response will not receive ConnectionClosed but instead wait for the timeout that is configured on their end.

[thomaseizinger]

Is io closed after upgrade_inbound anyway?

I will have to experiment with this a bit more but I couldn't get the code to trigger ConnectionClosed on the waiting end if I just drop the oneshot.

[romanb]

If the substream is not explicitely closed, the side waiting for the response will not receive ConnectionClosed but instead wait for the timeout that is configured on their end.

Every substream must be close()ed properly in write_response (and write_request, if you don't expect to receive a response). See e.g. #1606 w.r.t. what can happen otherwise. Indeed it may be a good idea to close().await in the InboundUpgrade of libp2p-request-response to avoid this pitfall. But note that this closes the substream, not the connection. So I'm a bit confused why you expect ConnectionClosed.

[romanb]

See also #1660.

[thomaseizinger]

If the substream is not explicitely closed, the side waiting for the response will not receive ConnectionClosed but instead wait for the timeout that is configured on their end.

Every substream must be close()ed properly in write_response (and write_request, if you don't expect to receive a response). See e.g. #1606 w.r.t. what can happen otherwise. Indeed it may be a good idea to close().await in the InboundUpgrade of libp2p-request-response to avoid this pitfall. But note that this closes the substream, not the connection. So I'm a bit confused why you expect ConnectionClosed.

Sorry I should have probably made that more clear!

The ConnectionClosed event I was referring to is this one:

rust-libp2p/protocols/request-response/src/lib.rs

Line 171 in 68587ee

ConnectionClosed,

I was assuming that this only refers to the substream and not the actual connection though.

I will double check if #1660 produces the behaviour I'd want :)

[thomaseizinger]

So I checked with #1660 applied but it unfortunately doesn't give me the result I am anticipating. I've also experimented with a local patch that moves the closing of the substream to the very end of the upgrade, applying it regardless of whether we sent a response or not.

Doing so produces the intended behaviour of the sender receiving a RequestResponseEvent::ConnectionClosed immediately and they don't have to wait for their configured timeout to occur.

I tried to write a test for it within the request-response crate but I couldn't get it to work with the provided Ping protocol. For some weird reason, not sending a response gets interpreted as an empty response on the sender side and they emit Response(Pong([])). See my patch here: 0c37129

[romanb]

Ok, I think I finally understand the point. If your protocol generally expects responses, as your example does, and you explicitly close the substream without sending a response, the receiving end will encounter a premature EOF when reading the response, and any error in an upgrade other than a timeout results in an error returned by the connection handler that will close the connection. Furthermore, while you can do this right now, you would prefer if this behaviour could be achieved by just dropping the ResponseChannel, which is possible if the InboundUpgrade always explicitly closes the substream at the end, regardless of whether a response is sent. I think that is fine, so I applied your suggestion in #1660. Note, however, that this (i.e. closing the connection) is a very rough way to report an error to the other peer: In my opinion it should only be done if the other peer seems misbehaved. To report any other kind of (often transient) errors, a request-response protocol should have proper error responses as part of its protocol.

[thomaseizinger]

Yes, thank you elaborating on it again!

I am aware that this kind of error handling is not very nice!
However, it is an IMO nice way of iterating on a protocol design. One can start simple and add explicit errors when necessary.

Thanks for considering the usecase and applying the suggestion! :)