(draft) jsonbuilder: better handling of memory allocation errors - v1#8847
(draft) jsonbuilder: better handling of memory allocation errors - v1#8847jasonish wants to merge 3 commits into
Conversation
Some very minor changes to formatting.
Convert "new_object" and "new_array" functions that return a Result and use "try_reserve" to allocate the amount of data requested. This should allow memory allocation errors to be detected and handled in a Rust-ful matter without resorting to catching a panic. Ticket: OISF#6057
Provide a wrapper around "push" and "push_str" on the internal buffer that will "try_reserve" data before growing in an attempt to handle memory allocation errors. Ticket: OISF#6057
|
@catenacyber Still draft, but I'd appreciate your thoughts on this. |
| /// return an error if unable to. | ||
| pub fn push_str(&mut self, s: &str) -> Result<&mut Self, JsonError> { | ||
| if self.buf.capacity() < self.buf.len() + s.len() { | ||
| self.buf.try_reserve(INIT_SIZE)?; |
There was a problem hiding this comment.
I don't like the doubling that many allocators do, but this should at least check try_reserve the max of INIT_SIZE or s.len()...
There was a problem hiding this comment.
There is no try_push, right ?
There was a problem hiding this comment.
There is no
try_push, right ?
No. But push, push_str won't allocated unless they need to, which they won't if we try_reserve first.
|
WARNING:
Pipeline 13704 |
Looks cool. |
You don't really. This is very much an active effort to opt-in to. The only thing I can think of is making the internal buffer harder to access, so you have to think twice about it. So open to ideas... I guess its a bit like return values in C.. Nothing forces you to, but its a good idea :) |
|
A more complete version is now up here: #8855 |
3 participants
Wrap (most) growth of the internal buffer in methods that first try to reserve
the required data and return a Result if unable to do so. The idea being we
return error on memory failures rather than panic/abort.
Ticket: https://redmine.openinfosecfoundation.org/issues/6057