-
Notifications
You must be signed in to change notification settings - Fork 29
feat: use opaque memory in HTTP APIs #684
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
base: main
Are you sure you want to change the base?
Changes from all commits
852a1b1
6024805
f679e1d
5170798
d465aba
d4b2b88
5da02f3
81c97f5
6a0d206
ca604d3
2876de0
0090821
ceac638
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
|
|
||
| #[cfg(not(test))] | ||
| use alloc::boxed::Box; | ||
| use core::any::{Any, TypeId}; | ||
|
|
||
| use thread_aware::ThreadAware; | ||
|
|
||
|
|
@@ -24,9 +25,21 @@ pub struct OpaqueMemory { | |
|
|
||
| impl OpaqueMemory { | ||
| /// Creates a new instance of the adapter. | ||
| /// | ||
| /// # Panics | ||
| /// | ||
| /// Panics only if runtime type identification reports [`OpaqueMemory`] but the downcast of | ||
| /// the same value to [`OpaqueMemory`] fails, which would indicate a standard library defect. | ||
| #[must_use] | ||
| pub fn new(inner: impl MemoryShared) -> Self { | ||
| Self { inner: Box::new(inner) } | ||
| pub fn new<M: MemoryShared>(inner: M) -> Self { | ||
| if TypeId::of::<M>() == TypeId::of::<Self>() { | ||
| let inner: Box<dyn Any> = Box::new(inner); | ||
| *inner | ||
| .downcast::<Self>() | ||
| .expect("the concrete type was verified as OpaqueMemory above") | ||
| } else { | ||
| Self { inner: Box::new(inner) } | ||
| } | ||
| } | ||
|
|
||
| /// Reserves at least `min_bytes` bytes of memory capacity. | ||
|
|
@@ -91,6 +104,16 @@ mod tests { | |
| assert!(builder.capacity() >= 1024); | ||
| } | ||
|
|
||
| #[test] | ||
| fn accepts_existing_opaque_memory() { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Posted by an AI agent The no-double-wrap regression test cannot distinguish nested wrappers Problem Why this matters Suggested fix |
||
| let memory = OpaqueMemory::new(GlobalPool::new()); | ||
| let memory = OpaqueMemory::new(memory); | ||
|
|
||
| let builder = memory.reserve(1024); | ||
|
|
||
| assert!(builder.capacity() >= 1024); | ||
| } | ||
|
|
||
| #[test] | ||
| fn memory_trait() { | ||
| let provider = GlobalPool::new(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,7 +17,7 @@ use std::borrow::Cow; | |
| use std::fmt::Debug; | ||
| use std::sync::Arc; | ||
|
|
||
| use bytesbuf::mem::GlobalPool; | ||
| use bytesbuf::mem::OpaqueMemory; | ||
| use http_extensions::{HttpBodyBuilder, RequestHandler}; | ||
| use opentelemetry::metrics::Meter; | ||
| use thread_aware::{PerCore, ThreadAware, unaware}; | ||
|
|
@@ -53,7 +53,7 @@ where | |
| /// Clock for timing operations and timeouts. | ||
| pub clock: Clock, | ||
| /// Memory pool for usage-neutral memory allocations. | ||
| pub global_pool: GlobalPool, | ||
| pub memory: OpaqueMemory, | ||
| /// Extra dependencies forwarded verbatim to [`CustomContext::extras`]. | ||
| pub extras: Extras, | ||
| } | ||
|
|
@@ -210,12 +210,12 @@ impl HttpClient { | |
| runtime_name: runtime.into(), | ||
| name: transport.into(), | ||
| clock: deps.clock.clone(), | ||
| global_pool: deps.global_pool.clone(), | ||
| memory: deps.memory.clone(), | ||
| isolation, | ||
| inner: thread_aware::Arc::new_with((deps, unaware(factory)), |(deps, factory)| { | ||
| Arc::new(move |options, meter, pool_index| { | ||
| let context = CustomContext { | ||
| body_builder: create_body_builder(&deps.global_pool, &deps.clock, &options), | ||
| body_builder: create_body_builder(&deps.memory, &deps.clock, &options), | ||
| clock: deps.clock.clone(), | ||
| pool_index, | ||
| extras: deps.extras.clone(), | ||
|
|
@@ -242,7 +242,7 @@ pub(crate) struct Transport { | |
| name: Cow<'static, str>, | ||
| inner: thread_aware::Arc<TransportFn, PerCore>, | ||
| clock: Clock, | ||
| global_pool: GlobalPool, | ||
| memory: OpaqueMemory, | ||
| isolation: Isolation, | ||
| } | ||
|
|
||
|
|
@@ -268,7 +268,7 @@ impl Transport { | |
| } | ||
|
|
||
| pub(crate) fn create_body_builder(&self, options: &ClientOptions) -> HttpBodyBuilder { | ||
| create_body_builder(&self.global_pool, &self.clock, options) | ||
| create_body_builder(&self.memory, &self.clock, options) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -278,7 +278,7 @@ impl Debug for Transport { | |
| } | ||
| } | ||
|
|
||
| pub(crate) fn create_body_builder(pool: &GlobalPool, clock: &Clock, options: &ClientOptions) -> HttpBodyBuilder { | ||
| pub(crate) fn create_body_builder(pool: &OpaqueMemory, clock: &Clock, options: &ClientOptions) -> HttpBodyBuilder { | ||
| HttpBodyBuilder::new(pool.clone(), clock).with_options(options.response_body_options) | ||
| } | ||
|
|
||
|
|
@@ -288,9 +288,11 @@ mod tests { | |
| use std::sync::Arc; | ||
| use std::sync::atomic::{AtomicUsize, Ordering}; | ||
|
|
||
| use bytesbuf::BytesBuf; | ||
| use bytesbuf::mem::{GlobalPool, Memory, OpaqueMemory}; | ||
| use http::StatusCode; | ||
| use http_extensions::FakeHandler; | ||
| use thread_aware::unaware; | ||
| use thread_aware::{ThreadAware, unaware}; | ||
|
|
||
| use super::{CustomContext, CustomDeps, Isolation, create_builder}; | ||
| use crate::HttpResponseBuilder; | ||
|
|
@@ -301,7 +303,7 @@ mod tests { | |
| fn custom_deps() -> CustomDeps { | ||
| CustomDeps { | ||
| clock: FakeDeps::default().clock, | ||
| global_pool: bytesbuf::mem::GlobalPool::new(), | ||
| memory: bytesbuf::mem::OpaqueMemory::new(bytesbuf::mem::GlobalPool::new()), | ||
| extras: (), | ||
| } | ||
| } | ||
|
|
@@ -311,6 +313,36 @@ mod tests { | |
| FakeHandler::from_fn(|_req| HttpResponseBuilder::new_fake().status(StatusCode::OK).build()) | ||
| } | ||
|
|
||
| #[derive(Clone, Debug, ThreadAware)] | ||
| struct CustomMemory { | ||
| inner: GlobalPool, | ||
| } | ||
|
|
||
| impl Memory for CustomMemory { | ||
| fn reserve(&self, min_bytes: usize) -> BytesBuf { | ||
| self.inner.reserve(min_bytes) | ||
| } | ||
| } | ||
|
|
||
| #[cfg_attr(miri, ignore)] | ||
| #[tokio::test] | ||
| async fn custom_deps_accept_custom_opaque_memory() { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Posted by an AI agent · Non-blocking The custom-provider test does not prove provider wiring Problem Why this matters Suggested fix |
||
| let deps = CustomDeps { | ||
| clock: FakeDeps::default().clock, | ||
| memory: OpaqueMemory::new(CustomMemory { inner: GlobalPool::new() }), | ||
| extras: (), | ||
| }; | ||
|
|
||
| let client = create_builder("test-runtime", "test", ok_factory, Isolation::Shared, deps) | ||
| .insecure_allow_http() | ||
| .minimal_pipeline() | ||
| .build(); | ||
|
|
||
| let response = client.post("http://example.com").text("custom pool").fetch().await.unwrap(); | ||
|
|
||
| assert_eq!(response.status(), StatusCode::OK); | ||
| } | ||
|
|
||
| #[cfg_attr(miri, ignore)] | ||
| #[tokio::test] | ||
| async fn create_builder_serves_requests_through_custom_pipeline() { | ||
|
|
@@ -348,7 +380,7 @@ mod tests { | |
| let counter = Arc::new(AtomicUsize::new(0)); | ||
| let deps = CustomDeps { | ||
| clock: FakeDeps::default().clock, | ||
| global_pool: bytesbuf::mem::GlobalPool::new(), | ||
| memory: bytesbuf::mem::OpaqueMemory::new(bytesbuf::mem::GlobalPool::new()), | ||
| extras: unaware(Arc::clone(&counter)), | ||
| }; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,7 +33,7 @@ pub struct TokioDeps { | |
| /// Clock for timing operations and timeouts. | ||
| pub clock: Clock, | ||
| /// Memory pool for usage-neutral memory allocations. | ||
| pub global_pool: bytesbuf::mem::GlobalPool, | ||
| pub global_pool: bytesbuf::mem::OpaqueMemory, | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Posted by an AI agent · Non-blocking The Tokio dependency field understates the provider contract Problem Why this matters Suggested fix |
||
| } | ||
|
|
||
| impl Default for TokioDeps { | ||
|
|
@@ -47,7 +47,7 @@ impl TokioDeps { | |
| #[must_use] | ||
| pub fn with_clock(clock: &Clock) -> Self { | ||
| Self { | ||
| global_pool: bytesbuf::mem::GlobalPool::new(), | ||
| global_pool: bytesbuf::mem::OpaqueMemory::new(bytesbuf::mem::GlobalPool::new()), | ||
| clock: clock.clone(), | ||
| } | ||
| } | ||
|
|
@@ -64,7 +64,7 @@ impl HttpClient { | |
| pub fn builder_tokio(deps: impl Into<TokioDeps>) -> HttpClientBuilder { | ||
| let deps = deps.into(); | ||
| let clock = deps.clock.clone(); | ||
| let global_pool = deps.global_pool.clone(); | ||
| let memory = deps.global_pool.clone(); | ||
|
|
||
| // Re-layer on top of the in-crate `builder_custom_internal` path: the | ||
| // full `TokioDeps` rides through `CustomDeps::extras` so that the | ||
|
|
@@ -77,7 +77,7 @@ impl HttpClient { | |
| Isolation::Shared, | ||
| CustomDeps { | ||
| clock, | ||
| global_pool, | ||
| memory, | ||
| extras: deps, | ||
| }, | ||
| ) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.