Close response stream even if response cannot be sent

[thomaseizinger]

This was already fixed at some point (#1660) but it seems that recent changes unfortunately undid this again.

I added a test that verifies that the connection is closed for the dialing party.

[mxinden]

Pull request looks good to me, just left 4 comments which I am fine with being ignored.

Thanks @thomaseizinger for putting in the work, especially with the test.

In case this is not urgent I would suggest to give @romanb a chance to review as well as Roman is most familiar with this code path.

[mxinden]

Not directly related to this pull request: I wonder whether we should handle the case where self.request_sender.send fails. As far as I can tell, this can only fail in case our internal tracking in RequestResponseHandler.inbound looses track of the receiving side. I would suggest adding a debug_assert here to catch future regressions. What do people think? (Happy to move this to a follow-up pull request.)

diff --git a/protocols/request-response/src/handler/protocol.rs b/protocols/request-response/src/handler/protocol.rs
index bff57058..6cee3469 100644
--- a/protocols/request-response/src/handler/protocol.rs
+++ b/protocols/request-response/src/handler/protocol.rs
@@ -101,16 +101,23 @@ where
         async move {
             let read = self.codec.read_request(&protocol, &mut io);
             let request = read.await?;
-            if let Ok(()) = self.request_sender.send((self.request_id, request)) {
-                if let Ok(response) = self.response_receiver.await {
-                    let write = self.codec.write_response(&protocol, &mut io, response);
-                    write.await?;
-                } else {
-                    return Ok(false)
-                }
+            match self.request_sender.send((self.request_id, request)) {
+                Ok(()) => {},
+                Err(_) => debug_assert!(true, "Expect request receiver to be alive."),
+            }
+
+            if let Ok(response) = self.response_receiver.await {
+                let write = self.codec.write_response(&protocol, &mut io, response);
+                write.await?;
+
+                io.close().await?;
+                // Response was sent. Indicate to handler to emit a `ResponseSent` event.
+                Ok(true)
+            } else {
+                io.close().await?;
+                // No response was sent. Indicate to handler to emit a `ResponseOmission` event.
+                Ok(false)
             }
-            io.close().await?;
-            Ok(true)
         }.boxed()
     }
 }

[romanb]

My reasoning for silently ignoring this case is that it simply means the handler has been dropped and the connection closed, so there is nothing more to do. If we want to put an assertion there to check that the upgrade is never polled without the handler still being alive and holding the receiver, that sounds fine to me. But I think a separate PR would indeed be better, as you suggested yourself.

[thomaseizinger]

If the handler has already been dropped, whatever we return here will not be delivered anyway correct?

Could we also return an error? Because that would make the handling more ergonomic.

[thomaseizinger]

I was actually bothered by the control flow of this function because it is not that straight-forward to follow. Your snippet gave me an idea. What do you think of 9f97d7d?

[thomaseizinger]

I've opted for returning an io::ErrorKind::ConnectionAborted. As far as I understand, this should never show up anywhere because the handler has already been dropped and it would be the one to receive this error in ProtocolsHandler::inject_dial_upgrade_error but I found the error variant to be descriptive of when this happens.

[mxinden]

Could we also return an error?

I would prefer the assertion above, given that it would surface the underlying issue (wrong assumption) compared to the error which would be silently ignored. I do agree that 9f97d7d improves the control flow over the status-quo, but I would argue that the diff I suggested above is even simpler, given that there is no additional function required.

I would suggest reverting 9f97d7d in this pull request and applying the diff above in a follow-up pull request.

@thomaseizinger would the above work for you? Sorry for the lengthy review.

[thomaseizinger]

but I would argue that the diff I suggested above is even simpler, given that there is no additional function required.

The reason I introduced the function was because I wanted a clear separation between the control flow of handling the upgrade and closing the substream.

This bug has only been re-introduced because we early return from the code that handles the upgrade. However, I guess one could argue that the introduced test should now cover this.

I'll drop the last commit and leave it up to you if you want to do any refactoring :)

[romanb]

I only left a small suggestion beside those already given by @mxinden. Otherwise looks good to me, thanks for catching this (partial) regression for the case where no response is sent!

[romanb]

Just to avoid misunderstandings: This looks ready to be merged now, right?

[mxinden]

Just to avoid misunderstandings: This looks ready to be merged now, right?

Yes. I will merge now and push a commit updating cargo tomls and changelogs. I will update #1988 shortly after.

[mxinden]

For the record, commit updating changelog: 67c465c

Thanks @thomaseizinger! I will follow up with another pull request as discussed.