protocols/request-response: Report successful request handling

[mxinden]

When failing to respond to an inbound request, the RequestResponse
NetworkBehaviour would emit an InboundFailure event. When succeeding
to respond to an inbound request nothing would be emitted.

With this commit the RequestResponse NetworkBehaviour emits a
InboundFinished event for all inbound requests. The event contains a
result describing whether handling the inbound request succeeded or
failed. This can for example be used for statistics, counting the number
of requests handled per second.

Note: The Throttled NetworkBehaviour handles two types of incoming
requests. (1) user level requests and (2) credit grants. In case
handling the latter fails the Throttled NetworkBehaviour reports
this failure. This can be unexpected as the user would receive a failure
event for a request it never tried to handle. With this commit the
confusion can be even larger as a user would receive a success event for
a request it never handled.

[romanb]

With this commit the RequestResponse NetworkBehaviour emits a InboundFinished event for all inbound requests.

Not quite. If the ResponseChannel is simply dropped, no event is emitted. This is due to the support for one-way protocols.

This can for example be used for statistics, counting the number of requests handled per second.

Can this not already be calculated by correlating the number of calls to send_response with emitted InboundFailures? An event emitted from inject_fully_negotiated_inbound does not really tell you much more than what you already know at the call-site of send_response - the given data is simply sent on the underlying I/O stream. If the inbound request has already timed out, there will be an InboundFailure and similarly if there should be an I/O error sending the response.

[romanb]

I just noticed that e.g. InboundFailure::ConnectionClosed is never emitted. It was introduced in #1726 and I guess emitted by inject_connection_closed() in the same way as for outbound requests, but later removed. There was even temporarily a ResponseSent event until d1388f1. So in the current state I/O errors on sending responses result in the connection being closed, but otherwise no further events. I'm not sure yet what the best direction is to take regarding error / success events for inbound requests.

[mxinden]

Can this not already be calculated by correlating the number of calls to send_response with emitted InboundFailures?

Yes, one could count the overall inbound requests and the failed inbound requests and derive the number of successful inbound requests. I would find having both an inbound failure and an inbound success event a better user experience, simplifying things on the user-side. As far as I can tell an additional inbound success event does not imply a performance penalty, but I might be wrong.

I'm not sure yet what the best direction is to take regarding error / success events for inbound requests.

With your comments above in mind I will have to put more thoughts into this as well.

At a first glance it seems not intuitive to return a InboundFailure for some but not all inbound request failures. At the same time, given that we don't track inbound requests at the NetworkBehaviour level, reporting e.g. ConnectionClosed seems non-trivial.

[mxinden]

With paritytech/substrate@707c1bd Substrate tracks the number of failed inbound requests as well as the time it took to build responses, similar to what was suggested above by @romanb.

I think this is good enough at least for now. In case we need more visibility into this logic we can still revive this pull request.