Restructure HttpServer's API

[smklein]

TL;DR: Split HttpServer in twain to make the state-machine logic a little better.

Old API:
❌ wait_for_shutdown
required an external "join_handle" - it never actually acted on the self object of the server, and there are no guarantees the provided JoinHandle matches the one from run.
❌ run can be invoked multiple times, which would cause a panic on the second invocation.

New API:
✔️ Invoking start consumes self, and may only be invoked once, creating a new HttpServer object which represents the running server.
✔️ The running HttpServer may only be terminated once by calling close (taking no arguments), which signals the server to stop immediately, and waits for it to exit.
✔️ The HttpServer implements Future, and may be await-ed to query the completion of the server.

[davepacheco]

Thanks for taking this on! I've got some thoughts below on the new shutdown interfaces.

Two other things to do once we're settled on the interface changes:

• This will obviously be a breaking change. Dropshot doesn't have much of a release process yet, but we do have several consumers now, so I want to start doing one. Could you draft a few sentences explaining to a consumer how to update their program for the new API? You can just leave that here in the PR and I'll add that to the changelog when I create one.
• Although this is a separate repo and we do have other consumers now, the expectation is that Dropshot is still pretty tightly coupled to oxide-api-prototype. Could you try updating oxide-api-prototype for these changes? That will be good validation that the new interface still works where we need it.

Thanks again!

[smklein]

Thanks for taking this on! I've got some thoughts below on the new shutdown interfaces.

Two other things to do once we're settled on the interface changes:

• This will obviously be a breaking change. Dropshot doesn't have much of a release process yet, but we do have several consumers now, so I want to start doing one. Could you draft a few sentences explaining to a consumer how to update their program for the new API? You can just leave that here in the PR and I'll add that to the changelog when I create one.

Here's my attempt:

HttpServer has been split into two objects, HttpServerStarter, and HttpServer.

• In the old implementation, HttpServer represented both a pending and running server. Callers were expected to invoke run() to begin execution of the old server.
• In the new implementation, HttpServerStarter may be used to construct a server, and HttpServer represents the running server. Invoking HttpServerStarter::start() creates and HttpServer object, which represents the new server.

HttpServer now implements Future, and does not directly expose a tokio::JoinHandle.

• In the old implementation, HttpServer returned a tokio::JoinHandle, and callers were expected to invoked wait_for_shutdown to await the completion of a server.
• In the new implementation, HttpServer implements Future, and may be await-ed directly.

As an example:

// Old Version:
let mut server = HttpServer::new( /* Arguments are the same between versions */ )
  .map_err(|error| format!("failed to start server: {}", error))?;

let server_task = server.run();
server.wait_for_shutdown(server_task).await;

// New Version
let server = HttpServerStarter::new( /* Arguments are the same between versions */ )
  .map_err(|error| format!("failed to start server: {}", error))?
  .start();

server.await;

• Although this is a separate repo and we do have other consumers now, the expectation is that Dropshot is still pretty tightly coupled to oxide-api-prototype. Could you try updating oxide-api-prototype for these changes? That will be good validation that the new interface still works where we need it.

Thanks again!

Sure, I have a branch here: oxidecomputer/omicron#34

Won't work on GitHub until this is merged, but locally, this passes all tests and simplifies the code slightly.

[davepacheco]

Thanks for doing this. This is much cleaner than what was in master before. This was probably some of the earliest code in Dropshot and some of my own earliest Rust code! I had a couple of small questions but they're not blockers.

Thanks also for updating oxide-api-prototype. That code look cleaner too.

Lastly, could you update CHANGELOG.adoc for this so that other people using Dropshot know how to update their code?

[smklein]

Before merging, I think it might be worthwhile having a discussion about the operation of the server future.
Currently, the server is implemented with an internal tokio::task that executes regardless of whether or not the server is awaited. From the documentation about futures, this is potentially a violation of expectations:

Futures alone are inert; they must be actively polled to make progress, meaning that each time the current task is woken up, it should actively re-poll pending futures that it still has an interest in.

I think it might make more sense to:

• Avoid calling tokio::spawn, and instead just use an (internal) Boxed future to represent the server.
• Only process requests for the server when server.await is invoked.

Although this wouldn't explicitly change the API, it would change expectations, in that it would require callers to explicitly await the server.
Currently, callers use a pattern like:

  let server = HttpServerStarter(...).start();

  // Users expect the server to be running here, without additional prompting.
  client.make_request().await;

  server.close().await;

However, removing the "spawned tokio task" from the internal representation would require users to do the following:

  let server = HttpServerStarter(...).start();

  // Users should not expect the server to respond to requests unless
  // it is await-ed (as it is here).
  select! {
    _ = client.make_request().fuse() => ...,
    _ = server => ...,
  }
  server.close().await;

Do you have an opinion here?

[davepacheco]

Before merging, I think it might be worthwhile having a discussion about the operation of the server future.
Currently, the server is implemented with an internal tokio::task that executes regardless of whether or not the server is awaited. From the documentation about futures, this is potentially a violation of expectations:

Futures alone are inert; they must be actively polled to make progress, meaning that each time the current task is woken up, it should actively re-poll pending futures that it still has an interest in.

I think it might make more sense to:

* Avoid calling `tokio::spawn`, and instead just use an (internal) Boxed future to represent the server.

* Only process requests for the server when `server.await` is invoked.

Although this wouldn't explicitly change the API, it would change expectations, in that it would require callers to explicitly await the server.
Currently, callers use a pattern like:

  let server = HttpServerStarter(...).start();

  // Users expect the server to be running here, without additional prompting.
  client.make_request().await;

  server.close().await;

However, removing the "spawned tokio task" from the internal representation would require users to do the following:

  let server = HttpServerStarter(...).start();

  // Users should not expect the server to respond to requests unless
  // it is await-ed (as it is here).
  select! {
    _ = client.make_request().fuse() => ...,
    _ = server => ...,
  }
  server.close().await;

Do you have an opinion here?

Good catch. This change feels like it would make the API unnecessarily harder to use. In practice I think most consumers will want this to be in a separate task. It seems annoying to make them all do that individually.

I hear what you're saying about this violating the principle of least surprise about futures, though. Maybe this is an argument for decoupling the server from the wait-for-shutdown future. start() could return the running server, but you'd have a separate function to wait for shutdown. I'm not sure if one would consider that particular future to be inert or not, but certainly it doesn't go off and do something without you awaiting on it.

As I understand it, this isn't so much a problem in master today because run() doesn't return a Future. It just starts the server and gives you a handle that you can use for shutting it down. (Of course, this interface still has all the problems you're solving in this PR. I think we're close here to the best of both worlds.)

[smklein]

Before merging, I think it might be worthwhile having a discussion about the operation of the server future.
Currently, the server is implemented with an internal tokio::task that executes regardless of whether or not the server is awaited. From the documentation about futures, this is potentially a violation of expectations:

Futures alone are inert; they must be actively polled to make progress, meaning that each time the current task is woken up, it should actively re-poll pending futures that it still has an interest in.

I think it might make more sense to:

* Avoid calling `tokio::spawn`, and instead just use an (internal) Boxed future to represent the server.

* Only process requests for the server when `server.await` is invoked.

Although this wouldn't explicitly change the API, it would change expectations, in that it would require callers to explicitly await the server.
Currently, callers use a pattern like:

  let server = HttpServerStarter(...).start();

  // Users expect the server to be running here, without additional prompting.
  client.make_request().await;

  server.close().await;

However, removing the "spawned tokio task" from the internal representation would require users to do the following:

  let server = HttpServerStarter(...).start();

  // Users should not expect the server to respond to requests unless
  // it is await-ed (as it is here).
  select! {
    _ = client.make_request().fuse() => ...,
    _ = server => ...,
  }
  server.close().await;

Do you have an opinion here?

Good catch. This change feels like it would make the API unnecessarily harder to use. In practice I think most consumers will want this to be in a separate task. It seems annoying to make them all do that individually.

¯\(ツ)/¯

I am a little split - I think that a "needs-to-be-polled-to-do-anything" future would be more verbose, but also more explicit.

I hear what you're saying about this violating the principle of least surprise about futures, though. Maybe this is an argument for decoupling the server from the wait-for-shutdown future. start() could return the running server, but you'd have a separate function to wait for shutdown. I'm not sure if one would consider that particular future to be inert or not, but certainly it doesn't go off and do something without you awaiting on it.

I'm not sure I understand this - I was thinking that we'd have start() return a running server, which would only complete once the server shuts down.

If we split the future in two, when would the "running server" future complete? Why would that be different from the "wait for future" case?

As I understand it, this isn't so much a problem in master today because run() doesn't return a Future. It just starts the server and gives you a handle that you can use for shutting it down. (Of course, this interface still has all the problems you're solving in this PR. I think we're close here to the best of both worlds.)

I think my takeaway here is going to be:

• Leave the interface as it currently exists in this PR
• If the implicitly running servers ends up being a problem later, we can deal with it in a follow-up PR. If not, then it's less verbose, so no problem.