upgrade to hyper v1 #1028

seanmonstar · 2024-05-28T13:30:09Z

This defines a dropshot::Body type that is used for both requests and
responses, in place of the now-gone hyper::Body.

Most of the changes are just renames. The interesting files (at least) are:

body.rs: This contains the new multi-purpose Body type. It has a few constructors to make things nice. Otherwise, it's not that complex, but could be tweaked or optimized with more usage and/or measurements.
server.rs: hyper got rid of the Accept trait. So now an accept loop is in this file manually. It also changed how graceful shutdown is coordinated, with a less-magical helper. It allows for adding a "graceful period timeout" if you eventually want.

jclulow · 2024-05-28T16:55:10Z

I currently make fairly extensive use of:

Body::wrap_stream(tokio_stream::wrappers::ReceiverStream::new(rx))

Will that still work with the new code?

seanmonstar · 2024-05-28T17:07:37Z

It'd be pretty straight forward to add such a constructor to dropshot::Body.

ahl

This is looking very promising. I'd love to get @sunshowers and @davepacheco to take a look. Can you please include the design document here? Also I would suggest taking a stab at migration content in the changelog as we try to help consumers figure out the path from previous versions. Thank you!

dropshot/src/body.rs

ahl · 2024-06-06T16:20:52Z

dropshot/src/body.rs

+impl Default for Body {
+    fn default() -> Body {
+        Body::empty()
+    }
+}


I think we may prefer people use ::empty() in order to be explicit about their intention... what do you think?

Adding a Default impl makes it easier to compose Body in other types that impl Default. For example, if you have this struct:

#[derive(Default)] struct Bodies { inner: HashMap<i32, dropshot::Body> }

You couldn't use the derive(Default) without the impl Default for Body here. You'd have to write a manual impl Default and set inner to HashMap::new().

Yeah true: can you envision a use case for that?

It's also used by Client::get(url), where it creates a "default" body that is assumed to be empty. And I believe that was used in some tests or docs.

In hindsight, default doesn't necessarily mean empty.

Yeah, I too prefer the explicitness of empty() over default() if the semantics are "an empty body". I went and looked at what would break if we just removed this Default impl and I see what you were saying @seanmonstar. It's stuff like:

error: could not compile `dropshot` (test "test_config") due to 9 previous errors warning: build failed, waiting for other jobs to finish... error[E0277]: the trait bound `dropshot::Body: Default` is not satisfied --> dropshot/tests/test_tls.rs:388:12 | 388 | client.get(uri.clone()).await.unwrap_err(); | ^^^ the trait `Default` is not implemented for `dropshot::Body` | note: required by a bound in `hyper_util::client::legacy::Client::<C, B>::get` --> /home/dap/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hyper-util-0.1.5/src/client/legacy/client.rs:164:12 | 162 | pub fn get(&self, uri: Uri) -> ResponseFuture | --- required by a bound in this associated function 163 | where 164 | B: Default, | ^^^^^^^ required by this bound in `Client::<C, B>::get`

Since GET requests must have an empty body, hyper's already implicitly assuming that "default" means "empty", and I see that hyper::Body in 0.14 impl'd Default in the same way. So I don't love this but it's probably not worse than before.

If we did want to avoid impl'ing Default here, would there be an easy way to fix up the tests? I don't really see one.

I can convert all calls to .get() to .request(req_with_explicitly_empty_body).

Ah, that's good to know. I could go either way on this -- @ahl what do you think?

dropshot/src/extractor/body.rs

dropshot/src/body.rs

dropshot/src/handler.rs

dropshot/src/server.rs

dropshot/src/body.rs

dropshot/src/http_util.rs

hawkw · 2024-06-07T21:17:18Z

dropshot/src/http_util.rs

+        let fr = maybefr?;
+        if let Ok(buf) = fr.into_data() {
+            nbytesread += buf.len();
+        }
    }

    // TODO-correctness why does the is_end_stream() assertion fail?


I think that the commented-out assertion here was probably failing because, after discarding all the DATA frames, an HTTP/2 body was terminated by a TRAILERS, so the stream was not EOF. I wonder if this assertion can be put back, now that the new implementation will also read and discard any TRAILERS frame that's sent as well as the DATA frames.

dropshot/src/server.rs

dropshot/tests/test_tls.rs

sunshowers

Thanks for your hard work on this!

dropshot/Cargo.toml

dropshot/src/body.rs

dropshot/src/server.rs

dropshot/src/test_util.rs

ahl · 2024-06-12T19:02:36Z

@seanmonstar this is looking great. I would like @davepacheco to take a pass before we merge it, but, @davepacheco, feel free to interpret that with whatever degree of rigor you consider appropriate.

davepacheco · 2024-06-13T19:58:46Z

Sorry I'm late to this one. I haven't had a chance to look yet. If there are breaking changes here, as it sounds like there are, then I'd like to get the changelog update in this PR. Our general approach is to provide super clear instructions for people to understand how to know if they're affected by a breaking change and exactly what changes to make, as concretely as we can. There are some examples here, from release 0.9.0.

@ahl also mentioned a design doc? That might help review?

davepacheco · 2024-06-13T20:04:34Z

I forgot to add: for breaking changes, I usually like to do a test update of one or more of our important consumers. We've found a lot of issues by doing that. Have we converted any consumers? Ideally it'd be great to convert Omicron's consumers -- is that too much to ask?

ahl · 2024-06-13T21:11:25Z

@ahl also mentioned a design doc? That might help review?

I followed up with dave privately with the document. We'll want to make sure it's recorded in this PR once we have appropriate ownership and permissions.

davepacheco

Thanks for doing this! I'm still working through server.rs but wanted to post these in the meantime.

dropshot/src/body.rs

davepacheco · 2024-06-13T20:21:24Z

dropshot/src/body.rs

+impl Default for Body {
+    fn default() -> Body {
+        Body::empty()
+    }
+}


Yeah, I too prefer the explicitness of empty() over default() if the semantics are "an empty body". I went and looked at what would break if we just removed this Default impl and I see what you were saying @seanmonstar. It's stuff like:

error: could not compile `dropshot` (test "test_config") due to 9 previous errors warning: build failed, waiting for other jobs to finish... error[E0277]: the trait bound `dropshot::Body: Default` is not satisfied --> dropshot/tests/test_tls.rs:388:12 | 388 | client.get(uri.clone()).await.unwrap_err(); | ^^^ the trait `Default` is not implemented for `dropshot::Body` | note: required by a bound in `hyper_util::client::legacy::Client::<C, B>::get` --> /home/dap/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hyper-util-0.1.5/src/client/legacy/client.rs:164:12 | 162 | pub fn get(&self, uri: Uri) -> ResponseFuture | --- required by a bound in this associated function 163 | where 164 | B: Default, | ^^^^^^^ required by this bound in `Client::<C, B>::get`

Since GET requests must have an empty body, hyper's already implicitly assuming that "default" means "empty", and I see that hyper::Body in 0.14 impl'd Default in the same way. So I don't love this but it's probably not worse than before.

If we did want to avoid impl'ing Default here, would there be an easy way to fix up the tests? I don't really see one.

dropshot/src/handler.rs

dropshot/src/test_util.rs

dropshot/src/websocket.rs

dropshot/tests/test_tls.rs

davepacheco · 2024-06-13T23:23:48Z

dropshot/src/server.rs

-        tokio::spawn(graceful)
+            loop {
+                tokio::select! {
+                    Ok((sock, remote_addr)) = self.0.accept() => {


What if accept() returns an error? The docs aren't super explicit but I gather this could happen any time the underlying accept(2) syscall returns an error, which can include transient errors like ECONNABORTED (on illumos, anyway). EMFILE might also be considered transient. (The docs also say "These would terminate the stream if not handled in any way." I'm not sure what that means. This struct is not a Stream and I don't know how it could know whether you handled the error.)

If this happens, according to the tokio::select! docs:

Once an returns a value, attempt to apply the value to the provided , if the pattern matches, evaluate and return. If the pattern does not match, disable the current branch and for the remainder of the current call to select!. Continue from step 3.

Step 3 waits for another of the futures to complete. So my interpretation of all of this is: if we encounter a transient error from accept(), this program will stop accepting new connections. This would be very bad.

Besides fixing this, it makes me a little nervous that we're implementing something so delicate and hard-to-test here. Is there some existing implementation we could use here? Would it help to turn this into a https://docs.rs/tokio-stream/0.1.15/tokio_stream/wrappers/struct.TcpListenerStream.html?

Good catch, hyper did handle all those before. But as part of the removal of the Accept trait, due to all of it's subtle issues, that was removed as well. Something generic might appear again in hyper-util, and when it does, Dropshot could use it.

For now, I can inline the behavior that used to exist.

I've added an HttpAcceptor type here that does what hyper used to do.

davepacheco · 2024-06-13T23:30:30Z

dropshot/src/server.rs

+                            TokioIo::new(sock),
+                            self.1.make_svc(remote_addr),
+                        );
+                        let fut = graceful.watch(fut.into_owned());


I'm having a hard time understanding the various futures involved here and how the new Graceful mechanism works. Would it be reasonable to add a comment describing this in more detail? And/or using more descriptive names?

I added some comments.

dropshot/src/server.rs

seanmonstar · 2024-06-17T20:23:06Z

I believe I've addressed all the feedback. I added some breaking changes notes to the changelog. Upgrading Omicron is currently out of scope for me.

davepacheco

Thanks for the updates.

davepacheco · 2024-06-21T17:48:31Z

dropshot/src/server.rs

+                    // These are errors on the individual socket that we
+                    // tried to accept, and so can be ignored.
+                    std::io::ErrorKind::ConnectionRefused
+                    | std::io::ErrorKind::ConnectionAborted
+                    | std::io::ErrorKind::ConnectionReset => (),
+
+                    // This could EMFILE implying resource exhaustion.
+                    // Sleep a little bit and try again.
+                    _ => {
+                        warn!(self.log, "accept error"; "error" => e);
+                        tokio::time::sleep(std::time::Duration::from_millis(
+                            100,
+                        ))
+                        .await;
+                    }


This is probably okay but I'm a little nervous about the error handling. This is definitely an area where the standards and manual pages are good but operational experience is also relevant, which is why I'm still a little uneasy about implementing this behavior here instead of using a hardened library. But I guess hyper 0.x was doing this inline before? Is this the same logic it used?

The ECONNABORTED check is definitely right and it's one a lot of software misses so thanks for getting that. ✔️

I checked out the illumos manual page (our production systems), POSIX, and Linux. I think most of the errors fall into one of these buckets:

"shouldn't ever happen and if they did you probably don't want to accept any more": EBADF, EFAULT, ENODEV, ENOTSOCK, EOPNOTSUPP, EPROTO, EINVAL, EAGAIN/EWOULDBLOCK

"can totally happen and should just be retried": ECONNABORTED, EINTR

"probably shouldn't happen, but definitely can, and there's probably not a great response but we could retry, and maybe pausing would help": EMFILE, ENOMEM, ENOSR, ENOBUFS, EPERM? (Linux)

I don't see ECONNREFUSED or ECONNRESET as possible errors that can be produced here, but the Linux manual page does say any other error for the new socket can happen here. That seems weird to me, but okay.

It seems brittle to encode all of these, so I guess the logic here is saying: assume the "can't happen" stuff really can't happen, handle ECONNABORTED[/ECONNREFUSED/ECONNRESET] explicitly as transient errors that don't require pausing, and treat everything else as a transient error, with a pause to avoid getting stuck in too tight a loop if we're wrong about it being transient. Is that the idea?

Correct, this is essentially the same code that hyper 0.14 was doing (minus some TCP keep-alive options stuff, which Dropshot didn't enable): https://github.com/hyperium/hyper/blob/0.14.x/src/server/tcp.rs#L211-L254

I do think it'd be nice to have this in hyper-util, so server frameworks don't all need a copy of this. There's a couple design questions to make it sufficiently generic (listener type, timer type, how to log, etc), such that I don't expect it to be ready to be depended on immediately.

dropshot/src/server.rs

davepacheco · 2024-06-21T17:53:15Z

dropshot/src/server.rs

@@ -327,6 +332,39 @@ impl<C: ServerContext> InnerHttpServerStarter<C> {
    }
 }

+/// This accepts connections more gracefully from a TcpListener.
+struct HttpAcceptor {


Now that we're incorporating this pretty tricky functionality into Dropshot, we should probably have some test coverage for it. Any ideas how to do that? It'd be really nice if we could exercise some of those error cases but that seems hard.

I did not have a way to programmatically trigger those error kinds. Maybe if it was sufficiently generic, such that some ErroringListener could be provided. But that won't be available yet.

Okay. How about the happy path? What about a test that starts a Dropshot server and makes 100 or 1000 connections to it in a row? Just thinking out loud about how best to test this automatically.

Yea I get that, I write a lot of tests and have multiple times refactored large test suites to make it easier to add more. In this case, I don't see much that could be tested. Especially since the accept code happy path is required for the rest of the unit tests to work, too. But I can add one accepting in a loop, like you mentioned.

dropshot/src/server.rs

CHANGELOG.adoc

davepacheco · 2024-06-21T21:21:23Z

I think the last items here are:

any tests for the HttpAcceptor, if we can think of something useful
test-converting Omicron, which @ahl is working on

…cer configuration (#1369) Trying to get my arms around syncing nexus and its satellites with oxidecomputer/dropshot#1028

This defines a `dropshot::Body` type that is used for both requests and responses, in place of the now-gone `hyper::Body`.

Co-authored-by: David Pacheco <dap@cs.brown.edu>

ahl · 2024-06-26T16:48:16Z

@seanmonstar it looks like Cargo.lock needs an upodate

ahl · 2024-06-27T21:16:12Z

@seanmonstar tests are failing

ahl · 2024-06-28T20:20:58Z

@seanmonstar looks like windows tests are hung?

ahl · 2024-07-01T16:08:11Z

Eyyyy! Passing tests! I'm working on merging this into omicron and its vassal repos; on vacation right now, but hope to shake out any kinks in a week or so.

CHANGELOG.adoc

ahl · 2024-09-26T18:54:23Z

WOOT

seanmonstar force-pushed the hyper-v1-2.4-own-body branch 2 times, most recently from ffe15a3 to 6e1ed6d Compare June 4, 2024 18:39

ahl reviewed Jun 6, 2024

View reviewed changes

ahl requested review from sunshowers and davepacheco June 6, 2024 17:38

hawkw reviewed Jun 7, 2024

View reviewed changes

sunshowers reviewed Jun 10, 2024

View reviewed changes

davepacheco reviewed Jun 13, 2024

View reviewed changes

ahl mentioned this pull request Jun 19, 2024

chore: bump reqwest to 0.12.5 oxidecomputer/progenitor#833

Closed

davepacheco reviewed Jun 21, 2024

View reviewed changes

This was referenced Jun 22, 2024

have oximeter-producer re-export dropshot types in its interface oxidecomputer/omicron#5933

Merged

update omicron deps; use re-exported dropshot types in oximeter-producer configuration oxidecomputer/crucible#1369

Merged

seanmonstar added 8 commits June 24, 2024 14:23

upgrade to hyper v1

b57c3f3

This defines a `dropshot::Body` type that is used for both requests and responses, in place of the now-gone `hyper::Body`.

remove ResponseBody alias

6e05fb7

remove unused line

da17e4c

remove commented out lines

96c093e

add a doc to Body

0140ee8

remove more commented out old code

12eb307

add comment about TcpListener::from_std

437f9f8

stick to hyper 1.3

57789e0

seanmonstar and others added 12 commits June 24, 2024 14:23

remove commented out assertion

009b864

remove unused Service<&TlsConn>

806be59

re-add WebsocketConnectionRaw

8b9ca26

fmt

79a92f4

change Body::full to Body::with_content

41deb68

handle tcp accept errors like hyper used to

e4fa983

add comments about graceful shutdown

31144df

add some breaking changes to changelog

02f7619

update CHANGELOG.adoc

e3f249c

Update dropshot/src/server.rs

b386977

Co-authored-by: David Pacheco <dap@cs.brown.edu>

remove possiblity of error from tcp accept

21f0aa0

add HttpAcceptor unit test

796918e

seanmonstar force-pushed the hyper-v1-2.4-own-body branch from 127a57d to 796918e Compare June 24, 2024 18:24

bump lockfile

209afa2

bless newer trybuild output

4e89c0a

use 127.0.0.1 in acceptor tests, to make windows happy

78310b4

Merge branch 'main' into hyper-v1-2.4-own-body

9075a2a

ahl mentioned this pull request Sep 14, 2024

migrate to hyper v1 oxidecomputer/omicron#6580

Merged

davepacheco reviewed Sep 19, 2024

View reviewed changes

CHANGELOG.adoc Outdated Show resolved Hide resolved

ahl added 4 commits September 26, 2024 10:59

Merge branch 'main' into hyper-v1-2.4-own-body

557a262

cargo.lock

6d0c5df

fix changelog

21769f2

update changelog again

35c897b

ahl merged commit 37dc307 into oxidecomputer:main Sep 26, 2024
10 checks passed

davepacheco mentioned this pull request Sep 27, 2024

commonize HTTP/HTTPS paths and use more structured errors internally #1121

Merged

upgrade to hyper v1 #1028

upgrade to hyper v1 #1028

Conversation

seanmonstar commented May 28, 2024

jclulow commented May 28, 2024

seanmonstar commented May 28, 2024

ahl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamchalmers Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunshowers left a comment • edited Loading

Choose a reason for hiding this comment

ahl commented Jun 12, 2024

davepacheco commented Jun 13, 2024

davepacheco commented Jun 13, 2024

ahl commented Jun 13, 2024

davepacheco left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanmonstar commented Jun 17, 2024

davepacheco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davepacheco commented Jun 21, 2024

ahl commented Jun 26, 2024

ahl commented Jun 27, 2024

ahl commented Jun 28, 2024

ahl commented Jul 1, 2024

ahl commented Sep 26, 2024

adamchalmers Jun 6, 2024 •

edited

Loading

sunshowers left a comment •

edited

Loading

davepacheco left a comment •

edited

Loading