Skip to content

Exposing ServerConfig options #959

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.

Sign up for GitHub

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Exposing ServerConfig options #959

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
9e2e063
Remove ServerConfig
augustuswm Apr 5, 2024
6e9e0f1
Fmt
augustuswm Apr 5, 2024
49723bf
Cleanup
augustuswm Apr 25, 2024
8637cde
Revert ownership change
augustuswm Apr 25, 2024
264a1ef
Fix doc
augustuswm Apr 25, 2024
9788f79
Fmt
augustuswm Apr 25, 2024
40ba009
Add breaking change note
augustuswm Apr 25, 2024
d3bbf4c
Bring up to main
augustuswm Aug 21, 2024
737d45b
Update changelog
augustuswm Aug 21, 2024
07b84de
Fmt
augustuswm Aug 21, 2024
a1609d8
Revert test change
augustuswm Aug 21, 2024
7d0851d
Update breaking change note
augustuswm Aug 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion dropshot/examples/basic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ async fn main() -> Result<(), String> {

// Set up the server.
let server =
HttpServerStarter::new(&config_dropshot, api, api_context, &log)
HttpServerStarter::new(config_dropshot, api, api_context, &log)
.map_err(|error| format!("failed to create server: {}", error))?
.start();

Expand Down
2 changes: 1 addition & 1 deletion dropshot/examples/file_server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ async fn main() -> Result<(), String> {
let context = FileServerContext { base: PathBuf::from(".") };

// Set up the server.
let server = HttpServerStarter::new(&config_dropshot, api, context, &log)
let server = HttpServerStarter::new(config_dropshot, api, context, &log)
.map_err(|error| format!("failed to create server: {}", error))?
.start();

Expand Down
2 changes: 1 addition & 1 deletion dropshot/examples/https.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ async fn main() -> Result<(), String> {

// Set up the server.
let server = HttpServerStarter::new_with_tls(
&config_dropshot,
config_dropshot,
api,
api_context,
&log,
Expand Down
2 changes: 1 addition & 1 deletion dropshot/examples/index.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ async fn main() -> Result<(), String> {
api.register(index).unwrap();

// Set up the server.
let server = HttpServerStarter::new(&config_dropshot, api, (), &log)
let server = HttpServerStarter::new(config_dropshot, api, (), &log)
.map_err(|error| format!("failed to create server: {}", error))?
.start();

Expand Down
2 changes: 1 addition & 1 deletion dropshot/examples/module-basic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ async fn main() -> Result<(), String> {

// Set up the server.
let server =
HttpServerStarter::new(&config_dropshot, api, api_context, &log)
HttpServerStarter::new(config_dropshot, api, api_context, &log)
.map_err(|error| format!("failed to create server: {}", error))?
.start();

Expand Down
12 changes: 4 additions & 8 deletions dropshot/examples/module-shared-context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,10 @@ async fn main() -> Result<(), String> {
let api_context = Arc::new(ExampleContext::new());

// Set up the server.
let server = HttpServerStarter::new(
&config_dropshot,
api,
api_context.clone(),
&log,
)
.map_err(|error| format!("failed to create server: {}", error))?
.start();
let server =
HttpServerStarter::new(config_dropshot, api, api_context.clone(), &log)
.map_err(|error| format!("failed to create server: {}", error))?
.start();

// Wait for the server to stop. Note that there's not any code to shut down
// this server, so we should never get past this point.
Expand Down
2 changes: 1 addition & 1 deletion dropshot/examples/multipart.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ async fn main() -> Result<(), String> {
api.register(index).unwrap();

// Set up the server.
let server = HttpServerStarter::new(&config_dropshot, api, (), &log)
let server = HttpServerStarter::new(config_dropshot, api, (), &log)
.map_err(|error| format!("failed to create server: {}", error))?
.start();

Expand Down
2 changes: 1 addition & 1 deletion dropshot/examples/multiple-servers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ impl SharedMultiServerContext {
log: log.clone(),
};
let server =
HttpServerStarter::new(&config_dropshot, api, context, &log)
HttpServerStarter::new(config_dropshot, api, context, &log)
.map_err(|error| format!("failed to create server: {}", error))?
.start();
let shutdown_handle = server.wait_for_shutdown();
Expand Down
2 changes: 1 addition & 1 deletion dropshot/examples/pagination-basic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ async fn main() -> Result<(), String> {
.map_err(|error| format!("failed to create logger: {}", error))?;
let mut api = ApiDescription::new();
api.register(example_list_projects).unwrap();
let server = HttpServerStarter::new(&config_dropshot, api, ctx, &log)
let server = HttpServerStarter::new(config_dropshot, api, ctx, &log)
.map_err(|error| format!("failed to create server: {}", error))?
.start();
server.await
Expand Down
2 changes: 1 addition & 1 deletion dropshot/examples/pagination-multiple-resources.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ async fn main() -> Result<(), String> {
api.register(example_list_projects).unwrap();
api.register(example_list_disks).unwrap();
api.register(example_list_instances).unwrap();
let server = HttpServerStarter::new(&config_dropshot, api, ctx, &log)
let server = HttpServerStarter::new(config_dropshot, api, ctx, &log)
.map_err(|error| format!("failed to create server: {}", error))?
.start();

Expand Down
2 changes: 1 addition & 1 deletion dropshot/examples/pagination-multiple-sorts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ async fn main() -> Result<(), String> {
.map_err(|error| format!("failed to create logger: {}", error))?;
let mut api = ApiDescription::new();
api.register(example_list_projects).unwrap();
let server = HttpServerStarter::new(&config_dropshot, api, ctx, &log)
let server = HttpServerStarter::new(config_dropshot, api, ctx, &log)
.map_err(|error| format!("failed to create server: {}", error))?
.start();

Expand Down
2 changes: 1 addition & 1 deletion dropshot/examples/request-headers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ async fn main() -> Result<(), String> {

let api_context = ();
let server =
HttpServerStarter::new(&config_dropshot, api, api_context, &log)
HttpServerStarter::new(config_dropshot, api, api_context, &log)
.map_err(|error| format!("failed to create server: {}", error))?
.start();
server.await
Expand Down
2 changes: 1 addition & 1 deletion dropshot/examples/self-referential.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ async fn main() -> Result<(), String> {

// Set up the server.
let server =
HttpServerStarter::new(&config_dropshot, api, api_context, &log)
HttpServerStarter::new(config_dropshot, api, api_context, &log)
.map_err(|error| format!("failed to create server: {}", error))?
.start();
let shutdown = server.wait_for_shutdown();
Expand Down
2 changes: 1 addition & 1 deletion dropshot/examples/websocket.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ async fn main() -> Result<(), String> {
api.register(example_api_websocket_counter).unwrap();

// Set up the server.
let server = HttpServerStarter::new(&config_dropshot, api, (), &log)
let server = HttpServerStarter::new(config_dropshot, api, (), &log)
.map_err(|error| format!("failed to create server: {}", error))?
.start();

Expand Down
2 changes: 1 addition & 1 deletion dropshot/examples/well-tagged.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ async fn main() -> Result<(), String> {
api.register(get_fryism).unwrap();

// Set up the server.
let server = HttpServerStarter::new(&config_dropshot, api, (), &log)
let server = HttpServerStarter::new(config_dropshot, api, (), &log)
.map_err(|error| format!("failed to create server: {}", error))?
.start();

Expand Down
7 changes: 7 additions & 0 deletions dropshot/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
use serde::Deserialize;
use serde::Serialize;
use std::net::SocketAddr;
use std::num::NonZeroU32;
use std::path::PathBuf;

/// Raw [`rustls::ServerConfig`] TLS configuration for use with
Expand Down Expand Up @@ -49,6 +50,10 @@ pub struct ConfigDropshot {
pub bind_address: SocketAddr,
/// maximum allowed size of a request body, defaults to 1024
pub request_body_max_bytes: usize,
/// maximum size of any page of results
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to add #[non_exhaustive] to the struct? Would that force consumers to use ..Default::default() when constructing this? i.e. such that adding more fields would not be a breaking change?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wondered the same thing. Consumers can already do that if they want. It's kind of nice that the way it is now you can have your code break if we add some new config option so that you get a chance to look yourself at whether our default works for you. I think I've used this in the past.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have to fix this here but I think I disagree with past-me here. If you want to do that, just check the changelog. We shouldn't need to bump a major to add new things.

A builder interface would be nice, too.

(I don't propose that we do anything in this PR.)

pub page_max_nitems: NonZeroU32,
/// default size for a page of results
pub page_default_nitems: NonZeroU32,
/// Default behavior for HTTP handler functions with respect to clients
/// disconnecting early.
pub default_handler_task_mode: HandlerTaskMode,
Expand Down Expand Up @@ -106,6 +111,8 @@ impl Default for ConfigDropshot {
ConfigDropshot {
bind_address: "127.0.0.1:0".parse().unwrap(),
request_body_max_bytes: 1024,
page_max_nitems: NonZeroU32::new(10000).unwrap(),
page_default_nitems: NonZeroU32::new(100).unwrap(),
default_handler_task_mode: HandlerTaskMode::Detached,
}
}
Expand Down
6 changes: 4 additions & 2 deletions dropshot/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
//! use dropshot::ConfigLoggingLevel;
//! use dropshot::HandlerTaskMode;
//! use dropshot::HttpServerStarter;
//! use std::sync::Arc;
//! use std::{num::NonZeroU32, sync::Arc};
//!
//! #[tokio::main]
//! async fn main() -> Result<(), String> {
Expand All @@ -68,9 +68,11 @@
//! // Start the server.
//! let server =
//! HttpServerStarter::new(
//! &ConfigDropshot {
//! ConfigDropshot {
//! bind_address: "127.0.0.1:0".parse().unwrap(),
//! request_body_max_bytes: 1024,
//! page_max_nitems: NonZeroU32::new(10000).unwrap(),
//! page_default_nitems: NonZeroU32::new(100).unwrap(),
//! default_handler_task_mode: HandlerTaskMode::Detached,
//! },
//! api,
Expand Down
47 changes: 11 additions & 36 deletions dropshot/src/server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ use std::convert::TryFrom;
use std::future::Future;
use std::mem;
use std::net::SocketAddr;
use std::num::NonZeroU32;
use std::panic;
use std::pin::Pin;
use std::sync::Arc;
Expand Down Expand Up @@ -63,7 +62,7 @@ pub struct DropshotState<C: ServerContext> {
/// caller-specific state
pub private: C,
/// static server configuration parameters
pub config: ServerConfig,
pub config: ConfigDropshot,
/// request router
pub router: HttpRouter<C>,
/// server-wide log handle
Expand All @@ -83,21 +82,6 @@ impl<C: ServerContext> DropshotState<C> {
}
}

/// Stores static configuration associated with the server
/// TODO-cleanup merge with ConfigDropshot
#[derive(Debug)]
pub struct ServerConfig {
/// maximum allowed size of a request body
pub request_body_max_bytes: usize,
/// maximum size of any page of results
pub page_max_nitems: NonZeroU32,
/// default size for a page of results
pub page_default_nitems: NonZeroU32,
/// Default behavior for HTTP handler functions with respect to clients
/// disconnecting early.
pub default_handler_task_mode: HandlerTaskMode,
}

pub struct HttpServerStarter<C: ServerContext> {
app_state: Arc<DropshotState<C>>,
local_addr: SocketAddr,
Expand All @@ -107,7 +91,7 @@ pub struct HttpServerStarter<C: ServerContext> {

impl<C: ServerContext> HttpServerStarter<C> {
pub fn new(
config: &ConfigDropshot,
config: ConfigDropshot,
api: ApiDescription<C>,
private: C,
log: &Logger,
Expand All @@ -116,27 +100,19 @@ impl<C: ServerContext> HttpServerStarter<C> {
}

pub fn new_with_tls(
config: &ConfigDropshot,
config: ConfigDropshot,
api: ApiDescription<C>,
private: C,
log: &Logger,
tls: Option<ConfigTls>,
) -> Result<HttpServerStarter<C>, GenericError> {
let server_config = ServerConfig {
// We start aggressively to ensure test coverage.
request_body_max_bytes: config.request_body_max_bytes,
page_max_nitems: NonZeroU32::new(10000).unwrap(),
page_default_nitems: NonZeroU32::new(100).unwrap(),
default_handler_task_mode: config.default_handler_task_mode,
};

let handler_waitgroup = WaitGroup::new();
let starter = match &tls {
Some(tls) => {
let (starter, app_state, local_addr) =
InnerHttpsServerStarter::new(
config,
server_config,
// server_config,
api,
private,
log,
Expand All @@ -154,7 +130,7 @@ impl<C: ServerContext> HttpServerStarter<C> {
let (starter, app_state, local_addr) =
InnerHttpServerStarter::new(
config,
server_config,
// server_config,
api,
private,
log,
Expand Down Expand Up @@ -274,8 +250,7 @@ impl<C: ServerContext> InnerHttpServerStarter<C> {
/// of `HttpServerStarter` (and await the result) to actually start the
/// server.
fn new(
config: &ConfigDropshot,
server_config: ServerConfig,
config: ConfigDropshot,
api: ApiDescription<C>,
private: C,
log: &Logger,
Expand All @@ -286,7 +261,8 @@ impl<C: ServerContext> InnerHttpServerStarter<C> {

let app_state = Arc::new(DropshotState {
private,
config: server_config,
// config: server_config,
config,
router: api.into_router(),
log: log.new(o!("local_addr" => local_addr)),
local_addr,
Expand Down Expand Up @@ -559,8 +535,7 @@ impl<C: ServerContext> InnerHttpsServerStarter<C> {
}

fn new(
config: &ConfigDropshot,
server_config: ServerConfig,
config: ConfigDropshot,
api: ApiDescription<C>,
private: C,
log: &Logger,
Expand All @@ -587,7 +562,7 @@ impl<C: ServerContext> InnerHttpsServerStarter<C> {

let app_state = Arc::new(DropshotState {
private,
config: server_config,
config,
router: api.into_router(),
log: logger,
local_addr,
Expand Down Expand Up @@ -1115,7 +1090,7 @@ mod test {
let log_context = LogContext::new("test server", &config_logging);
let log = &log_context.log;

let server = HttpServerStarter::new(&config_dropshot, api, 0, log)
let server = HttpServerStarter::new(config_dropshot, api, 0, log)
.unwrap()
.start();

Expand Down
4 changes: 2 additions & 2 deletions dropshot/src/test_util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,7 @@ impl<Context: ServerContext> TestContext<Context> {
pub fn new(
api: ApiDescription<Context>,
private: Context,
config_dropshot: &ConfigDropshot,
config_dropshot: ConfigDropshot,
log_context: Option<LogContext>,
log: Logger,
) -> TestContext<Context> {
Expand All @@ -492,7 +492,7 @@ impl<Context: ServerContext> TestContext<Context> {

// Set up the server itself.
let server =
HttpServerStarter::new(&config_dropshot, api, private, &log)
HttpServerStarter::new(config_dropshot, api, private, &log)
.unwrap()
.start();

Expand Down
9 changes: 5 additions & 4 deletions dropshot/src/websocket.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,10 +296,10 @@ impl JsonSchema for WebsocketUpgrade {
mod tests {
use crate::config::HandlerTaskMode;
use crate::router::HttpRouter;
use crate::server::{DropshotState, ServerConfig};
use crate::server::DropshotState;
use crate::{
ExclusiveExtractor, HttpError, RequestContext, RequestInfo,
WebsocketUpgrade,
ConfigDropshot, ExclusiveExtractor, HttpError, RequestContext,
RequestInfo, WebsocketUpgrade,
};
use debug_ignore::DebugIgnore;
use http::Request;
Expand All @@ -324,12 +324,13 @@ mod tests {
let rqctx = RequestContext {
server: Arc::new(DropshotState {
private: (),
config: ServerConfig {
config: ConfigDropshot {
request_body_max_bytes: 0,
page_max_nitems: NonZeroU32::new(1).unwrap(),
page_default_nitems: NonZeroU32::new(1).unwrap(),
default_handler_task_mode:
HandlerTaskMode::CancelOnDisconnect,
..Default::default()
},
router: HttpRouter::new(),
log: log.clone(),
Expand Down
2 changes: 1 addition & 1 deletion dropshot/tests/common/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ pub fn test_setup_with_context<Context: ServerContext>(

let logctx = create_log_context(test_name);
let log = logctx.log.new(o!());
TestContext::new(api, ctx, &config_dropshot, Some(logctx), log)
TestContext::new(api, ctx, config_dropshot, Some(logctx), log)
}

pub fn create_log_context(test_name: &str) -> LogContext {
Expand Down
Loading