tonic: Server::default(): Set TCP_NODELAY to true; improve docs (#2413)

## Motivation

I recently ran into problems where we accidentally "lost" the
tcp_nodelay=true setting, due to refactoring some code to make it easier
to test. This caused mysterious 40 ms latency increases. I hope this
makes it less likely for others to make this mistake.

## Solution

The documentation of the `Server::tcp_nodelay()` function said "Enabled
by default", but that was only true when using `Server::builder()`. The
`default()` method set this to false. To fix this:

* Change Server::default() to set tcp_nodelay: true.
* Change Server::builder() to just call Server::default().
* Add a test to verify the settings for nodelay and keepalive.
* Document the functions to note that the TCP settings are ignored when
using `serve_with_incoming`. `tcp_keepalive` already had this note. I
added the same documentation to `tcp_nodelay` and to
`serve_with_incoming` so it is less likely to be missed.

Author: evanj

tonic/src/transport/server/mod.rs

@@ -123,7 +123,7 @@ impl Default for Server<Identity> {
             init_connection_window_size: None,
             max_concurrent_streams: None,
             tcp_keepalive: None,
-            tcp_nodelay: false,
+            tcp_nodelay: true,
             http2_keepalive_interval: None,
             http2_keepalive_timeout: DEFAULT_HTTP2_KEEPALIVE_TIMEOUT,
             http2_adaptive_window: None,
@@ -148,11 +148,7 @@ pub struct Router<L = Identity> {
 impl Server {
     /// Create a new server builder that can configure a [`Server`].
     pub fn builder() -> Self {
-        Server {
-            tcp_nodelay: true,
-            accept_http1: false,
-            ..Default::default()
-        }
+        Self::default()
     }
 }
 
@@ -346,7 +342,7 @@ impl<L> Server<L> {
     /// specified will be the time to remain idle before sending TCP keepalive
     /// probes.
     ///
-    /// Important: This setting is only respected when not using `serve_with_incoming`.
+    /// Important: This setting is ignored when using `serve_with_incoming`.
     ///
     /// Default is no keepalive (`None`)
     ///
@@ -359,6 +355,8 @@ impl<L> Server<L> {
     }
 
     /// Set the value of `TCP_NODELAY` option for accepted connections. Enabled by default.
+    ///
+    /// Important: This setting is ignored when using `serve_with_incoming`.
     #[must_use]
     pub fn tcp_nodelay(self, enabled: bool) -> Self {
         Server {
@@ -607,6 +605,8 @@ impl<L> Server<L> {
     }
 
     /// Serve the service on the provided incoming stream.
+    ///
+    /// The `tcp_nodelay` and `tcp_keepalive` settings are ignored when using this method.
     pub async fn serve_with_incoming<S, I, IO, IE, ResBody>(
         self,
         svc: S,
@@ -1154,3 +1154,33 @@ where
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use std::time::Duration;
+
+    use crate::transport::Server;
+
+    #[test]
+    fn server_tcp_defaults() {
+        const EXAMPLE_TCP_KEEPALIVE: Duration = Duration::from_secs(10);
+
+        // Using ::builder() or ::default() should do the same thing
+        let server_via_builder = Server::builder();
+        assert!(server_via_builder.tcp_nodelay);
+        assert_eq!(server_via_builder.tcp_keepalive, None);
+        let server_via_default = Server::default();
+        assert!(server_via_default.tcp_nodelay);
+        assert_eq!(server_via_default.tcp_keepalive, None);
+
+        // overriding should be possible
+        let server_via_builder = Server::builder()
+            .tcp_nodelay(false)
+            .tcp_keepalive(Some(EXAMPLE_TCP_KEEPALIVE));
+        assert!(!server_via_builder.tcp_nodelay);
+        assert_eq!(
+            server_via_builder.tcp_keepalive,
+            Some(EXAMPLE_TCP_KEEPALIVE)
+        );
+    }
+}