Skip to content

Commit b2e0814

Browse files
committed
Cleanup code
1 parent 3f85700 commit b2e0814

13 files changed

Lines changed: 1004 additions & 867 deletions

File tree

crates/vespera/src/lib.rs

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,16 @@ impl<S> VesperaRouter<S>
8484
where
8585
S: Clone + Send + Sync + 'static,
8686
{
87-
/// Create a new `VesperaRouter` with a base router and routers to merge
87+
/// Create a `VesperaRouter` from a base router and the child-app router
88+
/// factories to merge into it.
89+
///
90+
/// This is invoked by the `vespera!` macro when the `merge = [...]`
91+
/// parameter is used; it is rarely constructed directly. Both the merge of
92+
/// the child routers and any [`layer`](Self::layer) added afterwards are
93+
/// **deferred** until [`with_state`](Self::with_state): Axum can only merge
94+
/// routers that share a state type, so the base router's state must be
95+
/// applied first. When a `vespera!` app has no `merge` entries the macro
96+
/// returns a plain `axum::Router` instead of this wrapper.
8897
#[must_use]
8998
pub fn new(base: axum::Router<S>, merge_fns: Vec<fn() -> axum::Router<()>>) -> Self {
9099
Self {

crates/vespera_core/src/openapi.rs

Lines changed: 11 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -225,25 +225,19 @@ impl OpenApi {
225225
self.external_docs = other.external_docs;
226226
}
227227

228-
// Merge tags (deduplicate by borrowed name). HashSets of borrowed
229-
// names avoid cloning existing tag names or incoming names solely for
230-
// indexing, while preserving first-wins and incoming insertion order.
228+
// Merge tags, de-duplicating by name in a single pass. `seen` starts
229+
// with the existing tag names and grows as incoming tags are appended,
230+
// so an incoming tag is kept only when its name collides with neither
231+
// an existing tag nor an already-appended incoming one (first-wins,
232+
// incoming insertion order preserved). Tag merging runs at compile
233+
// time over a handful of tags, so owning the names (one clone each)
234+
// is cheaper to read than the prior borrow-juggling two-pass flag Vec.
231235
if let Some(other_tags) = other.tags {
232236
let self_tags = self.tags.get_or_insert_with(Vec::new);
233-
let existing_names: std::collections::HashSet<&str> =
234-
self_tags.iter().map(|tag| tag.name.as_str()).collect();
235-
let mut incoming_names = std::collections::HashSet::new();
236-
let append_flags: Vec<_> = other_tags
237-
.iter()
238-
.map(|tag| {
239-
let name = tag.name.as_str();
240-
!existing_names.contains(name) && incoming_names.insert(name)
241-
})
242-
.collect();
243-
drop((existing_names, incoming_names));
244-
245-
for (tag, should_append) in other_tags.into_iter().zip(append_flags) {
246-
if should_append {
237+
let mut seen: std::collections::HashSet<String> =
238+
self_tags.iter().map(|tag| tag.name.clone()).collect();
239+
for tag in other_tags {
240+
if seen.insert(tag.name.clone()) {
247241
self_tags.push(tag);
248242
}
249243
}

crates/vespera_jni/src/jni_impl.rs

Lines changed: 18 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,8 @@ use crate::streaming_closures::{
1717
#[path = "jni_impl_streaming_buffer.rs"]
1818
mod streaming_buffer;
1919
use streaming_buffer::{
20-
StreamingBufferRole, checkout_streaming_chunk_buffer, mark_streaming_buffer_reusable,
20+
PullPushBuffers, StreamingBufferRole, checkout_pull_push_buffers,
21+
checkout_streaming_chunk_buffer, mark_streaming_buffer_reusable,
2122
};
2223

2324
/// Multi-threaded Tokio runtime shared across all JNI calls.
@@ -719,17 +720,14 @@ pub extern "system" fn Java_com_devfive_vespera_bridge_VesperaBridge_dispatchFul
719720
let jvm = env.get_java_vm()?;
720721

721722
// Pull and push run concurrently on different threads, so each
722-
// direction checks out its own per-thread cached buffer.
723-
let (pull_buf, pull_buf_lease) =
724-
checkout_streaming_chunk_buffer(env, StreamingBufferRole::Pull)?;
725-
let (push_buf, push_buf_lease) =
726-
match checkout_streaming_chunk_buffer(env, StreamingBufferRole::Push) {
727-
Ok(checked_out) => checked_out,
728-
Err(err) => {
729-
mark_streaming_buffer_reusable(pull_buf_lease);
730-
return Err(err);
731-
}
732-
};
723+
// direction checks out its own per-thread cached buffer (the
724+
// pull lease is released for us if the push checkout fails).
725+
let PullPushBuffers {
726+
pull_buf,
727+
pull_buf_lease,
728+
push_buf,
729+
push_buf_lease,
730+
} = checkout_pull_push_buffers(env)?;
733731

734732
// Closures capture clones of the JavaVM and Globals;
735733
// both types are Send+Sync.
@@ -904,17 +902,14 @@ pub extern "system" fn Java_com_devfive_vespera_bridge_VesperaBridge_dispatchFul
904902
let output_global: Global<JObject<'static>> = env.new_global_ref(&output_stream)?;
905903
let jvm = env.get_java_vm()?;
906904

907-
// Pull and push run concurrently on different threads.
908-
let (pull_buf, pull_buf_lease) =
909-
checkout_streaming_chunk_buffer(env, StreamingBufferRole::Pull)?;
910-
let (push_buf, push_buf_lease) =
911-
match checkout_streaming_chunk_buffer(env, StreamingBufferRole::Push) {
912-
Ok(checked_out) => checked_out,
913-
Err(err) => {
914-
mark_streaming_buffer_reusable(pull_buf_lease);
915-
return Err(err);
916-
}
917-
};
905+
// Pull and push run concurrently on different threads (the pull
906+
// lease is released for us if the push checkout fails).
907+
let PullPushBuffers {
908+
pull_buf,
909+
pull_buf_lease,
910+
push_buf,
911+
push_buf_lease,
912+
} = checkout_pull_push_buffers(env)?;
918913

919914
let pull_jvm = jvm.clone();
920915
let pull_global = input_global;

crates/vespera_jni/src/jni_impl_streaming_buffer.rs

Lines changed: 67 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -84,29 +84,38 @@ pub fn checkout_streaming_chunk_buffer(
8484
let size = streaming_chunk_size();
8585
role.with_cache(|cache| {
8686
let mut slot = cache.borrow_mut();
87-
let replace_cached = slot
88-
.as_ref()
89-
.is_none_or(|cached| cached.size != size && !cached.checked_out);
90-
91-
if replace_cached {
92-
*slot = Some(CachedStreamingChunkBuffer {
93-
size,
94-
array: new_streaming_chunk_buffer(env, size)?,
95-
checked_out: false,
96-
});
97-
}
98-
99-
let Some(cached) = slot.as_mut() else {
100-
return Ok((new_streaming_chunk_buffer(env, size)?, None));
101-
};
102-
103-
if cached.size != size || cached.checked_out {
104-
return Ok((new_streaming_chunk_buffer(env, size)?, None));
87+
// Three outcomes, decided by the cached slot's state:
88+
match slot.as_mut() {
89+
// Still checked out — a concurrent dispatch holds it, or a prior
90+
// dispatch panicked mid-stream and never returned its lease. Hand
91+
// back a throwaway, unpooled buffer and leave the cache untouched
92+
// so we never alias a Java array that may still be in flight.
93+
Some(cached) if cached.checked_out => {
94+
return Ok((new_streaming_chunk_buffer(env, size)?, None));
95+
}
96+
// Free to reuse — refresh the backing array only if the configured
97+
// chunk size changed, then lease it back to the caller.
98+
Some(cached) => {
99+
if cached.size != size {
100+
cached.array = new_streaming_chunk_buffer(env, size)?;
101+
cached.size = size;
102+
}
103+
let cached_array: &JByteArray<'static> = cached.array.as_ref();
104+
let dispatch_array = env.new_global_ref(cached_array)?;
105+
cached.checked_out = true;
106+
return Ok((dispatch_array, Some(StreamingChunkBufferLease::new(role))));
107+
}
108+
// Empty slot — fall through to install a fresh cached buffer.
109+
None => {}
105110
}
106-
107-
let cached_array: &JByteArray<'static> = cached.array.as_ref();
108-
let dispatch_array = env.new_global_ref(cached_array)?;
109-
cached.checked_out = true;
111+
let array = new_streaming_chunk_buffer(env, size)?;
112+
let array_ref: &JByteArray<'static> = array.as_ref();
113+
let dispatch_array = env.new_global_ref(array_ref)?;
114+
*slot = Some(CachedStreamingChunkBuffer {
115+
size,
116+
array,
117+
checked_out: true,
118+
});
110119
Ok((dispatch_array, Some(StreamingChunkBufferLease::new(role))))
111120
})
112121
}
@@ -116,3 +125,39 @@ pub fn mark_streaming_buffer_reusable(lease: Option<StreamingChunkBufferLease>)
116125
lease.mark_reusable();
117126
}
118127
}
128+
129+
/// The pull + push per-thread chunk buffers (and their leases) acquired
130+
/// together for one bidirectional streaming dispatch.
131+
pub struct PullPushBuffers {
132+
pub pull_buf: StreamingChunkBuffer,
133+
pub pull_buf_lease: Option<StreamingChunkBufferLease>,
134+
pub push_buf: StreamingChunkBuffer,
135+
pub push_buf_lease: Option<StreamingChunkBufferLease>,
136+
}
137+
138+
/// Check out the pull + push chunk buffers for a bidirectional stream in
139+
/// one step. Pull and push run concurrently on different threads, so each
140+
/// direction gets its own per-thread cached buffer.
141+
///
142+
/// If the push checkout fails after the pull buffer was already leased, the
143+
/// pull lease is released before returning the error so a half-acquired pair
144+
/// never leaks a leased buffer (which would force the next dispatch to
145+
/// allocate a fresh array). Centralising this cleanup keeps the invariant in
146+
/// one place instead of duplicating it across every bidirectional entry point.
147+
pub fn checkout_pull_push_buffers(env: &mut jni::Env<'_>) -> jni::errors::Result<PullPushBuffers> {
148+
let (pull_buf, pull_buf_lease) = checkout_streaming_chunk_buffer(env, StreamingBufferRole::Pull)?;
149+
let (push_buf, push_buf_lease) =
150+
match checkout_streaming_chunk_buffer(env, StreamingBufferRole::Push) {
151+
Ok(checked_out) => checked_out,
152+
Err(err) => {
153+
mark_streaming_buffer_reusable(pull_buf_lease);
154+
return Err(err);
155+
}
156+
};
157+
Ok(PullPushBuffers {
158+
pull_buf,
159+
pull_buf_lease,
160+
push_buf,
161+
push_buf_lease,
162+
})
163+
}

crates/vespera_macro/src/garde_emit.rs

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -156,14 +156,16 @@ fn emit_field_block(
156156
}
157157

158158
let field_name_str = field_ident.to_string();
159-
let numeric_kind = rust_numeric_kind(peel_option(field_ty).unwrap_or(field_ty));
159+
let numeric_kind = rust_numeric_kind(
160+
crate::schema_macro::type_utils::option_inner(field_ty).unwrap_or(field_ty),
161+
);
160162
let rule_blocks = emit_rule_blocks(c, &field_name_str, numeric_kind.as_deref());
161163
let dive_block = emit_dive_block(c);
162164
if rule_blocks.is_empty() && dive_block.is_empty() {
163165
return None;
164166
}
165167

166-
let block = if is_option_type(field_ty) {
168+
let block = if crate::schema_macro::type_utils::is_option_type(field_ty) {
167169
// `field_ident` is `&Option<T>` after the `let Self { .. } = self` destructure.
168170
// Match ergonomics make `inner` end up as `&T`.
169171
quote! {
@@ -397,16 +399,6 @@ fn numeric_some(value: Option<f64>, numeric_kind: Option<&str>) -> TokenStream {
397399
)
398400
}
399401

400-
#[cfg(feature = "validation")]
401-
fn is_option_type(ty: &Type) -> bool {
402-
crate::schema_macro::type_utils::option_inner(ty).is_some()
403-
}
404-
405-
#[cfg(feature = "validation")]
406-
fn peel_option(ty: &Type) -> Option<&Type> {
407-
crate::schema_macro::type_utils::option_inner(ty)
408-
}
409-
410402
#[cfg(feature = "validation")]
411403
fn rust_numeric_kind(ty: &Type) -> Option<String> {
412404
let Type::Path(tp) = ty else {

crates/vespera_macro/src/lib.rs

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -344,6 +344,14 @@ pub fn vespera(input: TokenStream) -> TokenStream {
344344
schema_macro::file_cache::bump_epoch();
345345

346346
let input = syn::parse_macro_input!(input as AutoRouterInput);
347+
// Capture the `dir = "..."` literal span (or the macro call site when
348+
// `dir` is omitted) before `process_vespera_input` consumes `input`, so a
349+
// "route folder not found" diagnostic points at the offending argument
350+
// rather than the whole `vespera!` invocation.
351+
let folder_span = input
352+
.dir
353+
.as_ref()
354+
.map_or_else(proc_macro2::Span::call_site, syn::LitStr::span);
347355
let processed = process_vespera_input(input);
348356
let schema_storage = SCHEMA_STORAGE
349357
.lock()
@@ -352,7 +360,7 @@ pub fn vespera(input: TokenStream) -> TokenStream {
352360
.lock()
353361
.unwrap_or_else(std::sync::PoisonError::into_inner);
354362

355-
match process_vespera_macro(&processed, &schema_storage, &route_storage) {
363+
match process_vespera_macro(&processed, &schema_storage, &route_storage, folder_span) {
356364
Ok(tokens) => tokens.into(),
357365
Err(e) => e.to_compile_error().into(),
358366
}
@@ -386,6 +394,12 @@ pub fn export_app(input: TokenStream) -> TokenStream {
386394
schema_macro::file_cache::bump_epoch();
387395

388396
let ExportAppInput { name, dir } = syn::parse_macro_input!(input as ExportAppInput);
397+
// Capture the `dir = "..."` literal span (or the macro call site when
398+
// `dir` is omitted) before `dir` is consumed below, so a "route folder
399+
// not found" diagnostic points at the offending argument.
400+
let folder_span = dir
401+
.as_ref()
402+
.map_or_else(proc_macro2::Span::call_site, syn::LitStr::span);
389403
let folder_name = dir
390404
.map(|d| d.value())
391405
.or_else(|| std::env::var("VESPERA_DIR").ok())
@@ -407,6 +421,7 @@ pub fn export_app(input: TokenStream) -> TokenStream {
407421
&schema_storage,
408422
&manifest_dir,
409423
&route_storage,
424+
folder_span,
410425
) {
411426
Ok(tokens) => tokens.into(),
412427
Err(e) => e.to_compile_error().into(),

0 commit comments

Comments
 (0)