Skip to content

fix(wit-bindgen-wrpc): clear out variant decoder internal state after use #257

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.

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

Conversation

vados-cosmonic
Copy link
Contributor

This commit fixes a bug which manifests itself when decoding a list of enums that may have separate internal decoder objects to use.

The issue is that storing the internal state inside the bindgen'd Decoder becomes a problem on the second (or subsequent) elements of a list of the given enum.

Given a vec![SomeEnum::X, SomeEnum::Y] the saved "state" for the first becomes the first decoder (i.e. the decoder for SomeEnum::X). When it comes time to parse the discriminant and bytes for Y the existence of the state causes a short circuit.

As the (wrong) decoder will generally try to process some bytes, you lose bytes from the input and then bytes are left over at the next attempt to decode (and then an error occurs). How this manifests is obviously specific to the enum, (wrong) decoder, and involved variants, how they are decoded -- i.e. where the decoding will fail.

This only happens for generating bindings for variant types (i.e. Rust enums), which use the PayloadDecoder machinery to dynamically pick which payload decoder to use, depending on the discriminant read.

@vados-cosmonic vados-cosmonic changed the title fix(wit-bindgen-wrpc): clear out internal state after use fix(wit-bindgen-wrpc): clear out variant decoder internal state after use Aug 21, 2024
rvolosatovs and others added 2 commits August 21, 2024 14:38
This commit fixes a bug which manifests itself when decoding a list of
enums that may have separate internal decoder objects to use.

The issue is that storing the internal state inside the bindgen'd
`Decoder` becomes a problem on the *second* (or subsequent) elements
of a list of the given enum.

Given a `vec![SomeEnum::X, SomeEnum::Y]` the saved "state" for the
first becomes the first decoder (i.e. the decoder for
`SomeEnum::X`). When it comes time to parse the discriminant and bytes
for `Y` the existence of the `state` causes a short circuit.

As the (wrong) decoder will generally try to process some bytes, you
lose bytes from the input and then bytes are left over at the next
attempt to decode (and then an error occurs). How this manifests is
obviously specific to the enum, (wrong) decoder, and involved
variants, how they are decoded -- i.e. where the decoding will fail.

This only happens for generating bindings for variant types (i.e. Rust
enums), which use the `PayloadDecoder` machinery to dynamically
pick *which* payload decoder to use, depending on the discriminant read.

Signed-off-by: Victor Adossi <[email protected]>
Copy link
Member

@rvolosatovs rvolosatovs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
I can confirm that this fixes the sync case, however this would break the async functionality (i.e. nested future or stream values in variants). I'll push a test case and a fix for the latter to this branch

@vados-cosmonic
Copy link
Contributor Author

Ah, so that's what the use case there was for -- to try to reuse an existing async decoder? Thanks for taking a look!

@rvolosatovs rvolosatovs force-pushed the fix(wit-bindgen-wrpc)=reuse-of-state-broken-list-of-enums branch from 8a433e3 to 4314bb2 Compare August 21, 2024 13:43
@rvolosatovs
Copy link
Member

Ah, so that's what the use case there was for -- to try to reuse an existing async decoder? Thanks for taking a look!

the decoders are generally meant to be reused indeed, which is why resetting their state is important once decoding succeeds (as was fixed in your commit)

Tokio interface is not sufficient for our use case, however, since we may also require deferred decoding of an async value after the synchronous part is done, which is what the Deferred trait handles. The deferred function (if any) must be preserved from the decoder state, which is what I've addressed in 4314bb2

@rvolosatovs rvolosatovs enabled auto-merge August 21, 2024 13:56
@rvolosatovs rvolosatovs added this pull request to the merge queue Aug 21, 2024
Merged via the queue into bytecodealliance:main with commit 1a19079 Aug 21, 2024
25 checks passed
@vados-cosmonic vados-cosmonic deleted the fix(wit-bindgen-wrpc)=reuse-of-state-broken-list-of-enums branch August 21, 2024 15:34
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