[request-response] Refine success & error reporting for inbound requests.

[romanb]

This PR is a follow-up to #1863 and #1860. Supersedes #1863.

Motivation

At the moment one can neither get confirmation when a response has been sent on the underlying transport, nor is one aware of response omissions. The latter was originally intended as a feature for support of one-way protocols, which seems like a bad idea in hindsight. The lack of a notification for sent responses may prohibit implementation of some request-response protocols that need to ensure a happens-before relation between sending a response and a subsequent request, besides possible uses for collecting statistics.

Change Summary

• Dropped support for "one-way" request-response protocols.
• New InboundFailure::ResponseOmission variant.
• Removed InboundFailure::ConnectionClosed variant.
• RequestResponse::send_response() now returns a Result, allowing early detection of failed inbound requests due to timeout or a closed connection.
• New RequestResponseEvent::ResponseSent variant, emitted when the response has been successfully written on the underlying transport connection.

Discussion

Even with these changes, there is no active notification for failed inbound requests as a result of connections unexpectedly closing, as is the case for outbound requests. Instead, for pending inbound requests this scenario can be identified, if necessary, either through errors returned on send_response or by the absence of both InboundFailure and ResponseSent events for a particular previously received request. Interest in this situation is not expected to be common and would otherwise require explicitly tracking all inbound requests in the RequestResponse behaviour just to actively report ConnectionClosed for each pending inbound request in that scenario, which would be a pity. Notably RequestResponse::send_response now also synchronously returns an error if the inbound upgrade handling the request has been aborted, due to timeout or closing of the connection, giving more options for graceful error handling for inbound requests. It is also noteworthy that client code that does not call send_response() will now necessarily receive a InboundFailure::ResponseOmission, so except for the case where a connection closes just after send_response() succeeded, client code is usually bound to notice a closed connection already on send_response().

Note: This PR does not prohibit (re-)adding InboundFailure::ConnectionClosed and dedicated inbound request tracking in the future, and neither would doing so make any changes proposed here obsolete. The above discussion is just to expand on why I think the need for doing so becomes even less likely.

Throttled Wrapper Credit Grants

As an aside, the Throttled wrapper now no longer emits inbound or outbound error events occurring in the context of sending credit requests or responses. This is in addition to not emitting ResponseSent events for ACK responses of credit grants. I think the credit requests and responses, including errors, should be completely opaque to the user. If credit messages keep failing, the effect on the public API should merely be that of back-pressure due to running out of credit.

[mxinden]

Thanks for putting more thoughts into this.

Dropped support for "one-way" request-response protocols.

I guess requiring an (empty) response is not much of a performance penalty but only a user experience loss, given the already existing overhead of a connection oriented protocol (TCP) and the multiplexing layer on top.

New InboundFailure::ResponseOmission variant.

I imagine this to be helpful when debugging.

Removed InboundFailure::ConnectionClosed variant.

👍

RequestResponse::send_response() now returns a Result, allowing early detection of failed inbound requests due to timeout or a closed connection.

I am in favor of this fast feedback.

New RequestResponseEvent::ResponseSent variant, emitted when the response has been successfully written on the underlying transport connection.

👍

[mxinden]

After various attempts, I didn't find a better way to be certain about the events emitted by the API than to track the request IDs relating to pending credit requests or responses. Thereby I also merged the credit_messages top-level hashmap keyed by PeerId into the peer_info map, as the former entries never live longer than the latter, aggregating all credit information with a peer for sending and receiving in the structs SendBudget and ReceiveBudget. I hope this is a small improvement. See 3c352cb.

Thanks for tackling this. Changes look good to me. I am in favor of merging credit_messages into peer_info.

[mxinden]

Any objects to merging this pull request? In case there are none, I would merge and release today and update paritytech/substrate#7478.

[romanb]

I think I just have some minor pending cleanup to do today. I will do that today and then merge and release.

[romanb]

Before preparing a new release I'd like to get a patch for #1872 in as well, hopefully still today.