fix(bin/client): don't close closing connection (mozilla#1866)

* fix(bin/client): don't close closing connection

The `bin/src/client/mod.rs` `Runner::run` function continuously checks whether
there is more work. In case there is none, it initiates closing of the
connection (`self.client.close`) and then `continue`s to the top of the loop in
order to send out a closing frame.

https://github.com/mozilla/neqo/blob/14cafbaa7fa88434def2c1d19e932c08e00173f8/neqo-bin/src/client/mod.rs#L376-L409

There is a potential busy loop when closing an already closing connection.
`Runner::run` will call `self.client.close` and then continously `continue` to
the top of the loop.

This commit differentiates a connection state in `NotClosing`, `Closing` and
`Closed`. It only attempts to close a `NotClosing` connection and only then
`continue`s to the top of the loop.

* Introduce ConnectionError::is_error and implement TryFrom

neqo-bin/src/client/http09.rs

@@ -25,7 +25,7 @@ use neqo_transport::{
 };
 use url::Url;
 
-use super::{get_output_file, qlog_new, Args, Res};
+use super::{get_output_file, qlog_new, Args, CloseState, Res};
 
 pub struct Handler<'a> {
     streams: HashMap<StreamId, Option<BufWriter<File>>>,
@@ -142,6 +142,26 @@ pub(crate) fn create_client(
     Ok(client)
 }
 
+impl TryFrom<&State> for CloseState {
+    type Error = ConnectionError;
+
+    fn try_from(value: &State) -> Result<Self, Self::Error> {
+        let (state, error) = match value {
+            State::Closing { error, .. } | State::Draining { error, .. } => {
+                (CloseState::Closing, error)
+            }
+            State::Closed(error) => (CloseState::Closed, error),
+            _ => return Ok(CloseState::NotClosing),
+        };
+
+        if error.is_error() {
+            Err(error.clone())
+        } else {
+            Ok(state)
+        }
+    }
+}
+
 impl super::Client for Connection {
     fn process_output(&mut self, now: Instant) -> Output {
         self.process_output(now)
@@ -163,15 +183,8 @@ impl super::Client for Connection {
         }
     }
 
-    fn is_closed(&self) -> Result<bool, ConnectionError> {
-        match self.state() {
-            State::Closed(
-                ConnectionError::Transport(neqo_transport::Error::NoError)
-                | ConnectionError::Application(0),
-            ) => Ok(true),
-            State::Closed(err) => Err(err.clone()),
-            _ => Ok(false),
-        }
+    fn is_closed(&self) -> Result<CloseState, ConnectionError> {
+        self.state().try_into()
     }
 
     fn stats(&self) -> neqo_transport::Stats {

neqo-bin/src/client/http3.rs

@@ -27,7 +27,7 @@ use neqo_transport::{
 };
 use url::Url;
 
-use super::{get_output_file, qlog_new, Args, Res};
+use super::{get_output_file, qlog_new, Args, CloseState, Res};
 
 pub(crate) struct Handler<'a> {
     #[allow(
@@ -105,17 +105,28 @@ pub(crate) fn create_client(
     Ok(client)
 }
 
-impl super::Client for Http3Client {
-    fn is_closed(&self) -> Result<bool, ConnectionError> {
-        match self.state() {
-            Http3State::Closed(
-                ConnectionError::Transport(neqo_transport::Error::NoError)
-                | ConnectionError::Application(0),
-            ) => Ok(true),
-            Http3State::Closed(err) => Err(err.clone()),
-            _ => Ok(false),
+impl TryFrom<Http3State> for CloseState {
+    type Error = ConnectionError;
+
+    fn try_from(value: Http3State) -> Result<Self, Self::Error> {
+        let (state, error) = match value {
+            Http3State::Closing(error) => (CloseState::Closing, error),
+            Http3State::Closed(error) => (CloseState::Closed, error),
+            _ => return Ok(CloseState::NotClosing),
+        };
+
+        if error.is_error() {
+            Err(error.clone())
+        } else {
+            Ok(state)
         }
     }
+}
+
+impl super::Client for Http3Client {
+    fn is_closed(&self) -> Result<CloseState, ConnectionError> {
+        self.state().try_into()
+    }
 
     fn process_output(&mut self, now: Instant) -> Output {
         self.process_output(now)

neqo-bin/src/client/mod.rs

@@ -345,6 +345,12 @@ trait Handler {
     fn take_token(&mut self) -> Option<ResumptionToken>;
 }
 
+enum CloseState {
+    NotClosing,
+    Closing,
+    Closed,
+}
+
 /// Network client, e.g. [`neqo_transport::Connection`] or [`neqo_http3::Http3Client`].
 trait Client {
     fn process_output(&mut self, now: Instant) -> Output;
@@ -355,11 +361,7 @@ trait Client {
     fn close<S>(&mut self, now: Instant, app_error: AppError, msg: S)
     where
         S: AsRef<str> + Display;
-    /// Returns [`Some(_)`] if the connection is closed.
-    ///
-    /// Note that connection was closed without error on
-    /// [`Some(ConnectionError::Transport(TransportError::NoError))`].
-    fn is_closed(&self) -> Result<bool, ConnectionError>;
+    fn is_closed(&self) -> Result<CloseState, ConnectionError>;
     fn stats(&self) -> neqo_transport::Stats;
 }
 
@@ -381,16 +383,19 @@ impl<'a, H: Handler> Runner<'a, H> {
                 continue;
             }
 
+            #[allow(clippy::match_same_arms)]
             match (handler_done, self.client.is_closed()?) {
                 // more work
                 (false, _) => {}
                 // no more work, closing connection
-                (true, false) => {
+                (true, CloseState::NotClosing) => {
                     self.client.close(Instant::now(), 0, "kthxbye!");
                     continue;
                 }
+                // no more work, already closing connection
+                (true, CloseState::Closing) => {}
                 // no more work, connection closed, terminating
-                (true, true) => break,
+                (true, CloseState::Closed) => break,
             }
 
             match ready(self.socket, self.timeout.as_mut()).await? {

neqo-transport/src/lib.rs

@@ -223,6 +223,16 @@ impl ConnectionError {
             Self::Transport(_) => None,
         }
     }
+
+    /// Checks enclosed error for [`Error::NoError`] and
+    /// [`ConnectionError::Application(0)`].
+    #[must_use]
+    pub fn is_error(&self) -> bool {
+        !matches!(
+            self,
+            ConnectionError::Transport(Error::NoError) | ConnectionError::Application(0),
+        )
+    }
 }
 
 impl From<CloseError> for ConnectionError {