Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

api proposal: add Result return value to Encode #288

Open
darkskygit opened this issue Apr 5, 2023 · 3 comments · May be fixed by #286
Open

api proposal: add Result return value to Encode #288

darkskygit opened this issue Apr 5, 2023 · 3 comments · May be fixed by #286

Comments

@darkskygit
Copy link

Motivation

When encoding crdt items into binary, unexpected data may be encountered. At this time, yrs will interrupt the execution process through panic! like this:

y-crdt/yrs/src/block.rs

Line 882 in f5f7792

panic!("Couldn't get item's parent")

The panic will cause the execution thread to crash, which makes the developer need to isolate the relevant calls of yrs through catch_unwind or other means in actual development.

Rust provides Result abstraction, which allows us to deal with errors more gently.

Currently, the Decode trait already supports returning Result, so developers can safely handle decoding errors when encountering erroneous data:

y-crdt/yrs/src/updates/decoder.rs

Lines 10 to 12 in f5f7792

pub trait Decode: Sized {
fn decode<D: Decoder>(decoder: &mut D) -> Result<Self, Error>;

Use Case

Before:

let doc = Doc::new();

// do some complex operations

// encode to binary, no error message, only panic when something goes wrong
let data = doc.transact().state_vector().encode_v1();

After:

let doc = Doc::new();

// do some complex operations

// encode to binary, catch error through `Result`
let data = doc.transact().state_vector().encode_v1()?;
// or
match doc.transact().state_vector().encode_v1() {
    Ok(data) => {
        // on success
    },
    Err(e) => {
        // on error
    }
}
@darkskygit darkskygit linked a pull request Apr 5, 2023 that will close this issue
Open
@Horusiath
Copy link
Collaborator

Tbh. if I remember correctly, the panic that you've quoted should be unreachable - all integrated blocks always have parent set, otherwise it's a bug.

We pretty much did what we can in order to catch error-prone data. There's also another reason for that - if you passed data that cannot be serialised into Doc it'll be stored there regardless. Any attempt to generate update from such document is always bound to fail (since we integrated non-serializable data into Doc state). So if there's any room for error, IMO it should be when a new piece of data is added.

Where errors could be useful in serialisation process is when the Encoder uses something different than Vec buffer underneath, that can potentially fail. But currently it's not a scenario, that anyone really proposes.

@darkskygit
Copy link
Author

darkskygit commented Apr 6, 2023 •
edited

Tbh. if I remember correctly, the panic that you've quoted should be unreachable - all integrated blocks always have parent set, otherwise it's a bug.

We pretty much did what we can in order to catch error-prone data. There's also another reason for that - if you passed data that cannot be serialised into Doc it'll be stored there regardless. Any attempt to generate update from such document is always bound to fail (since we integrated non-serializable data into Doc state). So if there's any room for error, IMO it should be when a new piece of data is added.

Where errors could be useful in serialisation process is when the Encoder uses something different than Vec buffer underneath, that can potentially fail. But currently it's not a scenario, that anyone really proposes.

In my practice, I only receive updates generated by Yjs and apply them to Doc instances in memory.

In some unknown scene(It usually happens after maintaining the connection in a client for more than 1 day. At present, some of our users are using edge or arc. They allow users to keep a tag active for a long time. At this time, the update generated by yjs will be sent to the server from time to time.), updates generated by Yjs can be successfully applied to Doc instances, but encoding them into binary fails - the usual failure is caused by "Couldn't get item's parent".

It sounds like what we should do more is to see if apply_update can do some checks on update to avoid abnormal crdt items being applied to doc? At present, apply_update also fails silently.

@darkskygit
Copy link
Author

darkskygit commented Apr 6, 2023 •
edited

The other benefit of making Encode return Result is that even if encoding fails, developers have a chance to recover, for example, by attempting to roll back the problematic update. In my experience, the updates that cause encoding failure are passed in from the remote web client. At present, there has never been a situation where the encoding failure caused by using yrs api to edit doc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

add Result for Encode trait toeverything/y-crdt
2 participants
@Horusiath @darkskygit