Skip to content

Add fallback for as_trusted_for calls#185

Closed
kskalski wants to merge 1 commit into
anza-xyz:masterfrom
kskalski:ks/as_trust_fall
Closed

Add fallback for as_trusted_for calls#185
kskalski wants to merge 1 commit into
anza-xyz:masterfrom
kskalski:ks/as_trust_fall

Conversation

@kskalski
Copy link
Copy Markdown
Contributor

Callers of as_trusted_for should handle cases where Reader implementation isn't able to provide full buffer of desired size immediately. They should fallback to not using trusted reader in that case (slower, but still functional path).

Current API is almost ready for this pattern, since as_trusted_for returns a Result and its documentation kind of imply that if reader cannot satisfy the request, it should return error. However falling back to different way of reading for all errors could be wrond, since some errors are logic errors or underlying IO errors, which should cause immediate break and propagating errors up.
For this purpose special ReadError variant is introduced to mark recoverable error for too large as_trusted_for request.

Additionally it's imaginable that readers would be able to fulfill a smaller request for trusted reader and caller could adjust the request to still avoid bounds for whatever buffer space is available in the reader. Thus the recoverable error contains usize with the limit of currently available "trusted bytes" in case caller wants to loop and pick fast / slow path depending on such size.

@cpubot
Copy link
Copy Markdown
Contributor

cpubot commented Feb 17, 2026

This is an interesting idea. My concern with this particular implementation / API is that it will make trusted usage more cumbersome to deal with. In particular, SchemaRead implementers will always have to be handle the possibility that calling as_trusted_for may error with TrustedSizeAvailableLimit and recover gracefully.

In this spirit of this direction, we could add an additional method on Reader -- something like trusted_max_window_size (naming TBD). That way, implementers could check the trusted window limit up front and perform a chunked read approach. If implementers do not implement the chunking approach, the reader could gracefully fall back to the default, non-trusted implementation (the BufReader impl does this). This would prevent arbitrary failures that are dependent on payload size.

@kskalski
Copy link
Copy Markdown
Contributor Author

In this spirit of this direction, we could add an additional method on Reader -- something like trusted_max_window_size (naming TBD). That way, implementers could check the trusted window

Right, I suppose this is equivalent to calling fill_buf(usize::MAX)?.len(), allowing impl to possibly return value more efficiently (or choose to return value that avoids any expensive ops / IO).

That value won't be a constant though, since size of available "readily-available" data is runtime dependent.

chunking approach, the reader could gracefully fall back to the default, non-trusted implementation (the BufReader impl does this).

Aha, so this means:

  • if you check for available size, get properly sized trusted reader and handle any need for chunked operation, you will obtain and use best performant reader
  • by default / if you didn't check for available size, you might get a slower, but functional trusted reader, no errors

@kskalski
Copy link
Copy Markdown
Contributor Author

I was wondering if we really need the "Trusted" machinery at the level of Reader trait. Conceptually the places where the caller wants to use a trusted reader should be able to get a slice from fill_buf or borrow_exact and just use that (wrapped with TrustedSliceReader*) to perform a series of reads without bounds checks.

From what I understand the difficulty lies in deciding whether the obtained slice should be temporarily (fill_buf) or permanently (borrow_exact) borrowed to support zero-copy borrows somewhere at the bottom.
Additionally, we currently do not detect cases where a user wants to deserialize a reference when using a reader without borrow support (e.g. cursor, see #197).

An idea - what if we:

  • add "requires_borrow" to TypeMeta
  • read will decide to use fill_buf or borrow_mut as separate branches on requires_borrow
  • eliminate Trusted and as_trusted_reader from the Reader trait and instead in above branches just directly wrap the [u8] slice with proper trusted reader
  • for the fill_buf case it would be a bit more natural for the caller to handle loop over slices smaller than requested on their side - but, maybe we could even support both approaches by providing trusted chunked slices reader (I guess the way such type needs to work is capture &mut impl Reader and call fill_buf() on each operation without strong the slice)

@kskalski
Copy link
Copy Markdown
Contributor Author

We discussed with Zach the alternative API to Trusted associated type in Reader / Writer. Since the usage of trusted reader is typically to obtain a slice of (usually small) static size (importantly, it can also be a series, but uniform sized, slices when operating on a collection) he proposed to use functions that return impl Iterator<Item = Chunk>.

Chunk is basically a &[u8] and we might actually use that as item's type. It's typically small (the maximum size is directly related to size_of::<LargestUsedStaticStruct>(), so user has reasonable control over it and misuse is discouraged e.g. by lints. This makes the case of chunked / buffered reader solvable by small memmove or aux buffer (those are necessary only when chunk crosses buffer boundary).

I guess we will still benefit from having trusted reader over such known-size slices. For example

struct Element {
   a: [u8; 12],
   b: u64,
   c: [u32; 3]
}
struct Elements(Vec<Element>);

will obtain chunks of 32 bytes, but when populating each Element we should also avoid bounds checks when obtaining bytes for each specific a, b, c fields.

In short - we could just return equivalent of TrustedSliceReader and TrustedSliceWriter or return slices, but expose the trusted readers to easily wrap the chunks.

In the meantime I will close this PR, since we have better solution on the horizon.

@kskalski kskalski closed this Feb 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants