Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: graceful shutdown after switch to hyper 1.x #779

Merged
merged 1 commit into from
Mar 27, 2024

Conversation

DDtKey
Copy link
Contributor

@DDtKey DDtKey commented Mar 24, 2024

It was removed after switch to hyper 1.x (commit) for some reason

Is there any reason? Because it seems to broke the logic (encountered issues after upgrade)

The changes affected the behavior
-    let mut conn = Http::new()
-        .serve_connection(socket, service)
-        .with_upgrades();
+    let builder = auto::Builder::new(hyper_util::rt::TokioExecutor::new());
+    let conn =
+        builder.serve_connection_with_upgrades(hyper_util::rt::TokioIo::new(socket), service);
+    futures_util::pin_mut!(conn);

     tokio::select! {
-        _ = &mut conn => {
+        _ = conn => {
             // Connection completed successfully.
             return;
         },
@@ -366,10 +368,4 @@
         }
         _ = server_graceful_shutdown_token.cancelled() => {}
     }
-
-    // Init graceful shutdown for connection (`GOAWAY` for `HTTP/2` or disabling `keep-alive` for `HTTP/1`)
-    Pin::new(&mut conn).graceful_shutdown();
-
-    // Continue awaiting after graceful-shutdown is initiated to handle existed requests.
-    let _ = conn.await;

For some reason it was removed after switch to `hyper 1.x` ([commit](poem-web@0b6ca89))

<details>
<summary> Patch affected the behavior </summary>

```patch
-    let mut conn = Http::new()
-        .serve_connection(socket, service)
-        .with_upgrades();
+    let builder = auto::Builder::new(hyper_util::rt::TokioExecutor::new());
+    let conn =
+        builder.serve_connection_with_upgrades(hyper_util::rt::TokioIo::new(socket), service);
+    futures_util::pin_mut!(conn);

     tokio::select! {
-        _ = &mut conn => {
+        _ = conn => {
             // Connection completed successfully.
             return;
         },
@@ -366,10 +368,4 @@
         }
         _ = server_graceful_shutdown_token.cancelled() => {}
     }
-
-    // Init graceful shutdown for connection (`GOAWAY` for `HTTP/2` or disabling `keep-alive` for `HTTP/1`)
-    Pin::new(&mut conn).graceful_shutdown();
-
-    // Continue awaiting after graceful-shutdown is initiated to handle existed requests.
-    let _ = conn.await;
```

</details>
@DDtKey DDtKey changed the title fix: graceful shutdown after switch to hyper 1.x fix: graceful shutdown after switch to hyper 1.x Mar 24, 2024
@DDtKey DDtKey mentioned this pull request Mar 25, 2024
@attila-lin
Copy link
Collaborator

Thanks!

@attila-lin attila-lin merged commit 6484545 into poem-web:master Mar 27, 2024
7 checks passed
DDtKey added a commit to DDtKey/poem that referenced this pull request Mar 27, 2024
For some reason it was removed after switch to `hyper 1.x` ([commit](poem-web@0b6ca89))

<details>
<summary> Patch affected the behavior </summary>

```patch
-    let mut conn = Http::new()
-        .serve_connection(socket, service)
-        .with_upgrades();
+    let builder = auto::Builder::new(hyper_util::rt::TokioExecutor::new());
+    let conn =
+        builder.serve_connection_with_upgrades(hyper_util::rt::TokioIo::new(socket), service);
+    futures_util::pin_mut!(conn);

     tokio::select! {
-        _ = &mut conn => {
+        _ = conn => {
             // Connection completed successfully.
             return;
         },
@@ -366,10 +368,4 @@
         }
         _ = server_graceful_shutdown_token.cancelled() => {}
     }
-
-    // Init graceful shutdown for connection (`GOAWAY` for `HTTP/2` or disabling `keep-alive` for `HTTP/1`)
-    Pin::new(&mut conn).graceful_shutdown();
-
-    // Continue awaiting after graceful-shutdown is initiated to handle existed requests.
-    let _ = conn.await;
```

</details>

(cherry picked from commit 6484545)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants