Skip to content

Commit 53f81e6

Browse files
committed
Rm clippy warning
1 parent 74cff02 commit 53f81e6

12 files changed

Lines changed: 215 additions & 55 deletions

File tree

crates/vespera_core/src/openapi.rs

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -316,12 +316,17 @@ impl OpenApi {
316316
// Merge tags, de-duplicating by name with first-wins semantics while
317317
// preserving deterministic output order (existing tags first, then
318318
// incoming tags in their original order).
319+
//
320+
// A linear `any` scan beats a `HashSet<String>` here: tag sets are
321+
// tiny (OpenAPI tags are top-level operation groupings — a handful,
322+
// rarely past a few dozen even for large APIs), so the O(n²) short-
323+
// string compare over an already-resident `Vec` is cheaper than
324+
// allocating a set and cloning every existing + incoming tag name.
325+
// Net: zero allocations and zero `String` clones on the merge path.
319326
if let Some(other_tags) = other.tags {
320327
let self_tags = self.tags.get_or_insert_with(Vec::new);
321-
let mut seen: std::collections::HashSet<String> =
322-
self_tags.iter().map(|tag| tag.name.clone()).collect();
323328
for tag in other_tags {
324-
if seen.insert(tag.name.clone()) {
329+
if !self_tags.iter().any(|existing| existing.name == tag.name) {
325330
self_tags.push(tag);
326331
}
327332
}

crates/vespera_inprocess/benches/dispatch/serde_ab.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,11 @@
88
//! `#[cfg(feature = "bench-support")]`). Wired into the parent `ab_benches`
99
//! criterion group.
1010
11+
// This is a `#[path]` bench submodule of `dispatch.rs`; it intentionally
12+
// re-uses the parent bench file's imports (criterion types, header types,
13+
// wire-assembly helpers) rather than re-listing them. The glob is the
14+
// idiomatic shape for a bench helper split out only to honour the file-size cap.
15+
#[allow(clippy::wildcard_imports)]
1116
use super::*;
1217

1318
/// `request_parse_*` / `response_serialize_*` within-run A/B: the hand-rolled

crates/vespera_inprocess/src/streaming.rs

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -543,13 +543,24 @@ where
543543
outcome
544544
}
545545

546+
/// Lock the producer handle, transparently recovering the guard if the mutex
547+
/// was poisoned. A poison here only means a prior holder panicked while the
548+
/// streaming path was tearing down; the guarded `Option<JoinHandle>` is still
549+
/// structurally valid, so recovering and proceeding is correct — and keeps this
550+
/// FFI-adjacent path free of the `unwrap` panic site each of the three call
551+
/// sites would otherwise carry. (Same idiom as the registry/bench read paths.)
552+
fn lock_producer_handle(
553+
producer_handle: &RequestProducerHandle,
554+
) -> std::sync::MutexGuard<'_, Option<tokio::task::JoinHandle<()>>> {
555+
producer_handle
556+
.lock()
557+
.unwrap_or_else(std::sync::PoisonError::into_inner)
558+
}
559+
546560
/// Whether the request producer task was started — i.e. the handler read
547561
/// at least one body chunk, which lazily spawns the producer.
548562
fn producer_was_started(producer_handle: &RequestProducerHandle) -> bool {
549-
match producer_handle.lock() {
550-
Ok(guard) => guard.is_some(),
551-
Err(poisoned) => poisoned.into_inner().is_some(),
552-
}
563+
lock_producer_handle(producer_handle).is_some()
553564
}
554565

555566
/// RAII guard that closes the request body source **exactly once** if the
@@ -750,24 +761,13 @@ fn store_request_producer_handle(
750761
producer_handle: &RequestProducerHandle,
751762
handle: tokio::task::JoinHandle<()>,
752763
) {
753-
match producer_handle.lock() {
754-
Ok(mut guard) => *guard = Some(handle),
755-
Err(poisoned) => {
756-
let mut guard = poisoned.into_inner();
757-
*guard = Some(handle);
758-
}
759-
}
764+
*lock_producer_handle(producer_handle) = Some(handle);
760765
}
761766

762767
async fn await_request_producer(producer_handle: &RequestProducerHandle) {
763-
let handle = match producer_handle.lock() {
764-
Ok(mut guard) => guard.take(),
765-
Err(poisoned) => {
766-
let mut guard = poisoned.into_inner();
767-
guard.take()
768-
}
769-
};
770-
768+
// Take the handle and release the guard on the same statement: a
769+
// `MutexGuard` is not `Send` and must never be held across the `.await`.
770+
let handle = lock_producer_handle(producer_handle).take();
771771
if let Some(handle) = handle {
772772
let _ = handle.await;
773773
}

crates/vespera_inprocess/src/wire/header_write.rs

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -182,15 +182,20 @@ fn write_headers<S: JsonSink>(sink: &mut S, headers: &http::HeaderMap) {
182182
return;
183183
}
184184
if key_count == 1 {
185-
let name = headers
186-
.keys()
187-
.next()
188-
.expect("keys_len()==1 yields exactly one name");
189-
sink.put(b"{");
190-
write_header_name_json_string(sink, name.as_str());
191-
sink.put(b":");
192-
write_header_value(sink, headers, name.as_str());
193-
sink.put(b"}");
185+
// `keys_len() == 1` guarantees exactly one key. On the impossible
186+
// `None` we emit an empty object instead of panicking, keeping this
187+
// FFI-adjacent response serializer free of unwind sites (mirrors the
188+
// no-panic/unwind discipline the dispatch internals document).
189+
if let Some(name) = headers.keys().next() {
190+
sink.put(b"{");
191+
write_header_name_json_string(sink, name.as_str());
192+
sink.put(b":");
193+
write_header_value(sink, headers, name.as_str());
194+
sink.put(b"}");
195+
} else {
196+
debug_assert!(false, "keys_len()==1 yields exactly one name");
197+
sink.put(b"{}");
198+
}
194199
return;
195200
}
196201

@@ -241,9 +246,14 @@ fn write_header_name_json_string<S: JsonSink>(sink: &mut S, name: &str) {
241246
/// (first, second, then the rest) — byte-identical, no second hash lookup.
242247
fn write_header_value<S: JsonSink>(sink: &mut S, headers: &http::HeaderMap, name: &str) {
243248
let mut values = headers.get_all(name).iter();
244-
let first = values
245-
.next()
246-
.expect("write_header_value is only called for present names");
249+
// `write_header_value` is only invoked for names taken from `headers.keys()`,
250+
// so `get_all(name)` is always non-empty. On the impossible `None` we emit an
251+
// empty-string value rather than panicking on this response hot path.
252+
let Some(first) = values.next() else {
253+
debug_assert!(false, "write_header_value is only called for present names");
254+
write_json_string(sink, "");
255+
return;
256+
};
247257
match values.next() {
248258
// Single value: emit the scalar string.
249259
None => write_json_string(sink, first.to_str().unwrap_or("")),

crates/vespera_jni/src/jni_impl.rs

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,7 @@ fn panic_wire() -> Vec<u8> {
199199
mod support;
200200
use support::{
201201
push_unless_header_failed, setup_full_stream, setup_full_stream_with_header, setup_stream,
202-
setup_stream_with_header, throw_streaming_abort,
202+
setup_stream_with_header, should_fire_fallback_header, throw_streaming_abort,
203203
};
204204

205205
/// `com.devfive.vespera.bridge.VesperaBridge.dispatchBytes(byte[]) -> byte[]`
@@ -684,8 +684,14 @@ pub extern "system" fn Java_com_devfive_vespera_bridge_VesperaBridge_dispatchStr
684684
}
685685
}
686686
Err(_) => {
687-
if !header_sent.load(Ordering::Relaxed)
688-
&& let Ok(fallback) = env.new_global_ref(&header_consumer)
687+
// See `should_fire_fallback_header`: a panic re-enters the
688+
// header consumer ONLY when it was never invoked (neither
689+
// succeeded nor threw), upholding the "invoked exactly once on
690+
// every code path" contract.
691+
if should_fire_fallback_header(
692+
header_sent.load(Ordering::Relaxed),
693+
header_failed.load(Ordering::Acquire),
694+
) && let Ok(fallback) = env.new_global_ref(&header_consumer)
689695
{
690696
let err = panic_wire();
691697
let _ = call_header_consumer(env, &fallback, &err);
@@ -828,8 +834,14 @@ pub extern "system" fn Java_com_devfive_vespera_bridge_VesperaBridge_dispatchFul
828834
}
829835
}
830836
Err(_) => {
831-
if !header_sent.load(Ordering::Relaxed)
832-
&& let Ok(fallback) = env.new_global_ref(&header_consumer)
837+
// See `should_fire_fallback_header`: a panic re-enters the
838+
// header consumer ONLY when it was never invoked (neither
839+
// succeeded nor threw), upholding the "invoked exactly once on
840+
// every code path" contract.
841+
if should_fire_fallback_header(
842+
header_sent.load(Ordering::Relaxed),
843+
header_failed.load(Ordering::Acquire),
844+
) && let Ok(fallback) = env.new_global_ref(&header_consumer)
833845
{
834846
let err = panic_wire();
835847
let _ = call_header_consumer(env, &fallback, &err);

crates/vespera_jni/src/jni_impl_streaming_abort_tests.rs

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use std::ops::ControlFlow;
22
use std::sync::atomic::{AtomicBool, Ordering};
33

4-
use super::push_unless_header_failed;
4+
use super::{push_unless_header_failed, should_fire_fallback_header};
55

66
#[test]
77
fn push_gate_aborts_without_writing_when_header_delivery_failed() {
@@ -49,3 +49,24 @@ fn push_gate_delegates_when_header_delivery_succeeded() {
4949
push_unless_header_failed(&header_failed, &mut |_| ControlFlow::Continue(()), b"x");
5050
assert!(stopped.is_break());
5151
}
52+
53+
#[test]
54+
fn fallback_header_fires_only_when_consumer_never_invoked() {
55+
// Panic unwound BEFORE the header callback was ever reached: the Java caller
56+
// has no header yet, so the one-shot 500 fallback MUST fire.
57+
assert!(should_fire_fallback_header(false, false));
58+
59+
// Header callback already SUCCEEDED: re-firing would deliver the header
60+
// twice — forbidden by the "invoked exactly once on every code path" contract.
61+
assert!(!should_fire_fallback_header(true, false));
62+
63+
// Header callback already THREW (it WAS invoked): a later panic must not
64+
// re-enter the (possibly broken / already-committed) consumer a second time.
65+
// This is the edge the prior `!header_sent`-only guard mishandled by
66+
// double-invoking the consumer.
67+
assert!(!should_fire_fallback_header(false, true));
68+
69+
// Defensive: both flags set never co-occurs in practice, but must still not
70+
// re-fire.
71+
assert!(!should_fire_fallback_header(true, true));
72+
}

crates/vespera_jni/src/jni_impl_support.rs

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,26 @@ pub(super) fn push_unless_header_failed(
3838
}
3939
}
4040

41+
/// Whether the panic-path fallback header (a one-shot `500`) should be delivered
42+
/// after a Rust panic unwound out of a streaming-with-header dispatch.
43+
///
44+
/// It fires ONLY when the header consumer was never invoked: `header_sent`
45+
/// records a SUCCESSFUL invocation and `header_failed` records one that THREW —
46+
/// either flag means "already invoked once", so a later panic must NOT re-enter
47+
/// the consumer. Re-entry would break the documented "header consumer invoked
48+
/// exactly once on every code path" contract and re-deliver to a consumer that
49+
/// may already be in a failed / partially-committed state. Only a panic that
50+
/// unwound BEFORE the callback was ever reached (both flags false) earns the
51+
/// fallback, so the Java caller is never left without a header.
52+
///
53+
/// (The prior inline guard tested `!header_sent` alone, which double-invoked the
54+
/// consumer in the rare "callback threw, then the dispatch future panicked"
55+
/// edge; this predicate closes that gap and is unit-tested in
56+
/// `jni_impl_streaming_abort_tests.rs`.)
57+
pub(super) fn should_fire_fallback_header(header_sent: bool, header_failed: bool) -> bool {
58+
!header_sent && !header_failed
59+
}
60+
4161
/// Promoted refs + a checked-out chunk buffer for a response
4262
/// streaming-with-header dispatch. Aliased so the helper return type stays
4363
/// under clippy's `type_complexity` cap.

libs/vespera-bridge/src/main/java/com/devfive/vespera/bridge/VesperaBridgeAutoConfiguration.java

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -138,8 +138,14 @@ public DispatchModeResolver vesperaBridgeDispatchModeResolver(VesperaBridgePrope
138138

139139
@Bean("vesperaBridgeAsyncResponseExecutor")
140140
@ConditionalOnMissingBean(name = "vesperaBridgeAsyncResponseExecutor")
141-
public ExecutorService vesperaBridgeAsyncResponseExecutor() {
142-
int threads = Math.max(2, Math.min(4, Runtime.getRuntime().availableProcessors()));
141+
public ExecutorService vesperaBridgeAsyncResponseExecutor(VesperaBridgeProperties props) {
142+
// Default (asyncPoolSize <= 0) preserves the historical sizing:
143+
// Math.max(2, Math.min(4, cpus)). A positive vespera.bridge.async-pool-size
144+
// overrides the cap for high-concurrency async dispatch (clamped to >= 1).
145+
int configured = props.getAsyncPoolSize();
146+
int threads = configured > 0
147+
? configured
148+
: Math.max(2, Math.min(4, Runtime.getRuntime().availableProcessors()));
143149
AtomicInteger seq = new AtomicInteger(1);
144150
ThreadFactory factory = task -> {
145151
Thread thread = new Thread(task, "vespera-bridge-async-response-" + seq.getAndIncrement());

libs/vespera-bridge/src/main/java/com/devfive/vespera/bridge/VesperaBridgeProperties.java

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,16 @@ public class VesperaBridgeProperties {
9090
*/
9191
private long maxBufferedRequestBytes = 0;
9292

93+
/**
94+
* Thread count for the autoconfigured {@code vesperaBridgeAsyncResponseExecutor}
95+
* — the JVM-side pool that parses the ASYNC wire response off the native
96+
* completion thread. Default {@code 0} preserves the historical sizing
97+
* ({@code Math.max(2, Math.min(4, availableProcessors()))}). Set a positive
98+
* value to override the cap for high-concurrency async dispatch; the value
99+
* is clamped to at least {@code 1}.
100+
*/
101+
private int asyncPoolSize = 0;
102+
93103
public String getAppHeader() {
94104
return appHeader;
95105
}
@@ -129,4 +139,12 @@ public long getMaxBufferedRequestBytes() {
129139
public void setMaxBufferedRequestBytes(long maxBufferedRequestBytes) {
130140
this.maxBufferedRequestBytes = maxBufferedRequestBytes;
131141
}
142+
143+
public int getAsyncPoolSize() {
144+
return asyncPoolSize;
145+
}
146+
147+
public void setAsyncPoolSize(int asyncPoolSize) {
148+
this.asyncPoolSize = asyncPoolSize;
149+
}
132150
}

libs/vespera-bridge/src/main/java/com/devfive/vespera/bridge/VesperaDirectBufferPool.java

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,16 @@ static boolean currentThreadIsVirtual() {
121121
}
122122
try {
123123
return (boolean) IS_VIRTUAL.invokeExact(Thread.currentThread());
124-
} catch (Throwable ignoredFallBackToPooled) {
124+
} catch (RuntimeException | Error fatalMustPropagate) {
125+
// JVM Errors (OutOfMemoryError, StackOverflowError, …) and runtime
126+
// exceptions are never the reflective-fallback case — let them
127+
// propagate instead of silently reporting "not virtual".
128+
throw fatalMustPropagate;
129+
} catch (Throwable reflectiveFailureFallBackToPooled) {
130+
// MethodHandle.invokeExact is declared `throws Throwable`; the only
131+
// residual checked failure here is a reflective/linkage problem
132+
// resolving Thread.isVirtual() — fall back to the non-virtual
133+
// (pooled) path, preserving the prior behavior.
125134
return false;
126135
}
127136
}
@@ -243,17 +252,17 @@ static ByteBuffer dispatchDirectPooled(
243252
// request (> cap): byte[] fallback is safe for any method
244253
// because no dispatch has run yet. The reusable header buffer
245254
// is consumed here, before any other fillHeaderJson call.
246-
return ByteBuffer.wrap(
247-
VesperaBridge.dispatchBytes(
248-
VesperaWireCodec.assembleWire(hdr.backingArray(), headerLen, bodyBytes)))
249-
.asReadOnlyBuffer();
255+
byte[] wire = VesperaWireCodec.assembleWire(hdr.backingArray(), headerLen, bodyBytes);
256+
VesperaWireCodec.shrinkHeaderBufferIfOversized(hdr);
257+
return ByteBuffer.wrap(VesperaBridge.dispatchBytes(wire)).asReadOnlyBuffer();
250258
}
251259
ByteBuffer[] pool = directPool();
252260
if (pool[0].capacity() < total) {
253261
pool[0] = ByteBuffer.allocateDirect(grownCapacity(total));
254262
}
255263
// Consume the reusable header buffer into the pooled direct buffer.
256264
int written = VesperaWireCodec.assembleInto(hdr.backingArray(), headerLen, bodyBytes, pool[0]);
265+
VesperaWireCodec.shrinkHeaderBufferIfOversized(hdr);
257266
if (written != total) {
258267
throw new IllegalStateException(
259268
"assembleInto wrote " + written + ", expected " + total);
@@ -289,16 +298,16 @@ static ByteBuffer dispatchDirectPooled(
289298
int headerLen = hdr.size();
290299
int total = VesperaWireCodec.wireTotalLength(headerLen, bodyBytes.length);
291300
if (currentThreadIsVirtual || total > DIRECT_MAX_CAPACITY) {
292-
return ByteBuffer.wrap(
293-
VesperaBridge.dispatchBytes(
294-
VesperaWireCodec.assembleWire(hdr.backingArray(), headerLen, bodyBytes)))
295-
.asReadOnlyBuffer();
301+
byte[] wire = VesperaWireCodec.assembleWire(hdr.backingArray(), headerLen, bodyBytes);
302+
VesperaWireCodec.shrinkHeaderBufferIfOversized(hdr);
303+
return ByteBuffer.wrap(VesperaBridge.dispatchBytes(wire)).asReadOnlyBuffer();
296304
}
297305
ByteBuffer[] pool = directPool();
298306
if (pool[0].capacity() < total) {
299307
pool[0] = ByteBuffer.allocateDirect(grownCapacity(total));
300308
}
301309
int written = VesperaWireCodec.assembleInto(hdr.backingArray(), headerLen, bodyBytes, pool[0]);
310+
VesperaWireCodec.shrinkHeaderBufferIfOversized(hdr);
302311
if (written != total) {
303312
throw new IllegalStateException(
304313
"assembleInto wrote " + written + ", expected " + total);

0 commit comments

Comments
 (0)