Expose reader publicly from `JsonStreamReader`

[kskalski]

Problem solved by the enhancement

I'm using in-memory bytes reader that I'm filling incrementally (this push-based model as opposed to std::io::Read's pull model has its uses and I'm able to utilize json stream reader in async context this way) as the stream parsing goes on, however the fact that JsonStreamReader takes ownership and hides access to the reader makes it inconvenient, requiring some technique for shared state access.
This essentially could be avoided if filling data into underlying reader doesn't need to be done concurrently with reading data from the stream reader and regular rules of Rust's memory access would be satisfied.

Enhancement description

Making reader field public would suffice, alternatively add pub fn reader(&self) -> &R and pub fn reader_mut(&mut self) -> &mut R accessors.

Alternatives / workarounds

Currently I'm using struct Handle(std::rc::Rc<std::cell::RefCell<R>>) implementing std::io::Read for it with dynamic borrows, but clearly that means adding a chunk of boilerplate code and sub-optimal performance.

[Marcono1234]

however the fact that JsonStreamReader takes ownership and hides access to the reader makes it inconvenient

The fact that it takes ownership is not the issue though, is it? Changing it to take a reference (if that is possible), wouldn't solve your problem, right? Because then you could not easily use the reader in any other way since the JsonStreamReader still has a mutable reference to it.

Your suggestion sounds reasonable to me. In case I will add this, I think I will go for the accessor methods; the public field approach might make refactoring in the future more difficult.

What would the use cases for the non-mut pub fn reader(&self) -> &R be? I guess there are some, but I am not sure if they are that common.

Also, for your use case I assume in addition to this enhancement request here you still need support for recovering from IO errors as mentioned in #21 (comment), right? So that after a WouldBlock error you obtain a reference to the reader, push more data to it later, and then retry.

[kskalski]

Yeah, basically lack of mutable access to the reader is an issue, so by ownership I meant that during lifetime of the json reader it borrows access to the underlying reader's memory even when no function is being called.

I suppose the need for read-only accessor is not that important, especially when mutable accessor would be available. It could have some use-cases like ability to check status of the reader, but since it has a concrete type known to the caller, it could be more, this simply reduces the need for mut context downstream for the caller. I would add it even if for the sake of symmetry / completeness when having mut accessor.

[kskalski]

Ability to recover from errors is a separate issue/feature. At the moment when processing websocket data I actually know from the protocol when top level values finish, so I can kind of avoid triggering EOF/WouldBlock errors, because it seems the streaming reader do not eagerly peek after the top level value stops until it is asked to parse next value.

For ability to parse partial values it could be useful to harden the reader in case it encounters errors, but it's more likely that the reader needs to be thrown away to recover parsing the full value, so it isn't necessary for this feature. Convenient access to the underlying reader simply allows me to cleanly control exactly what data is presented to the json reader and even rewind the consumption progress.
Currently I know the locations of top-level value endings due to the protocol I'm working with, but with proper access to "head" location from the json reader it would be even better (so then I could work with a concatenated byte stream without knowing the top-level value boundaries and infer them simply with json reader API).

[Marcono1234]

but it's more likely that the reader needs to be thrown away to recover parsing the full value

[...]

Convenient access to the underlying reader simply allows me to cleanly control exactly what data is presented to the json reader and even rewind the consumption progress.

Just to make sure I am not misunderstanding you here: You need access to the underlying reader while you are still using JsonStreamReader, and not after / when you throw the JsonStreamReader away due to partial values. Otherwise I assume you could work around this by providing a &Read to JsonStreamReader because &Read also implements Read. And then once JsonStreamReader goes out of scope you could access the original Read again.
But that is not possible for your use case, right?

I am a bit hesitant with this now because I had imagined such reader() methods mainly under the assumption that JsonStreamReader will also be able recover from IO errors (which it can't right now). So it feels to me a bit like your use case it quite specialized, but I might just be misunderstanding it.
If you want it would be great if you could show a small example how exactly you are using / planning to use JsonStreamReader, maybe also assuming that such a reader() method already exists.

[kskalski]

This feature is independent from partial reading, it just allows a more flexible model for using the API, i.e. allowing a push bytes mode of operation without locks. The basic example would be like:

    let mut json_reader = JsonStreamReader::new(VecDeque::new());
    json_reader.reader_mut().extend("{}".as_bytes());
    json_reader.has_next()?;

Passing reference is a problem, since first of all json reader needs a &mut Read not just &Read, and then if I provide mut reference, I can't use it to pupulate the buffer:

    let mut bytes = VecDeque::new();
    let mut json_reader = JsonStreamReader::new(&mut bytes);
    bytes.extend("{}".as_bytes());
    json_reader.has_next()?;

gives cannot borrow bytes as mutable more than once at a time

[kskalski]

As for the recovery case, yeah, making json reader operate on &mut Read and then re-using the buffer after the reader is discarded might work as you mentioned, but that assumes all data is already present in the buffer before JsonStreamReader is created, which might not be true - what this forces is throwing away reader and creating it again for each packet of bytes I want to add to the buffer.

[kskalski]

I'm no Rust expert, but I think its borrow checker and exclusive access rules make this pattern a reasonable approach, e.g. see this file from rustls.

[Marcono1234]

e.g. see this file from rustls

You mean the methods get_ref, get_mut and maybe into_parts there, right?

I have added JsonStreamReader::reader_mut() now, but no non-mut reader() method for now. The method is marked as experimental so that I can better understand how other users are using it.

Hopefully this solves your issue, though any additional feedback is appreciated!

[kskalski]

Thanks! Yes, my basic usage with new API is along the lines of:

pub struct RawWebsocket {
    ws: WsStream,
    json: struson::reader::JsonStreamReader<RingBuffer>,
}

impl RawWebsocket {
    /// Receive a JSON-deserializable message
    pub async fn recv_json<'a, T>(&mut self) -> Result<T, Error>
    where
        T: std::fmt::Debug + Deserialize<'a>,
    {
        if !self.json.reader_mut().has_data() {
            // loop until received complete message
            while self.ws.read_bytes(self.json.reader_mut()).await?.0 == 0 {}
        }
        let m = self.json.deserialize_next().unwrap();
        tracing::debug!("parsed {m:?}");
        Ok(m)
    }
}

so it seems to be sufficient.

I do think reader() and maybe into_parts() would make sense - letting do the above check if reader has data possible on & and then for partial parsing I might in fact need deconstructing the json reader without losing underlying buffer.

As for possibility to use &mut RingBuffer to instantiate json reader - it brings up a lot of mess with lifetimes, e.g. in code above I'm not even sure how to let json reader stored as field of struct to keep a mut reference to other field, rust is a bit limiting in that area.
But I suppose for partial parsing lots of that code would need to change, e.g. tracking data_pos and ensuring data since last parsed message is preserved, so it's likely that re-creation of the buffer from its internals on the side would be necessary anyway.