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.

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

Moving buffer.rewind() at the beginning of calls instead of end. #509

Closed
CedNaru opened this issue Oct 12, 2023 · 2 comments
Closed

Moving buffer.rewind() at the beginning of calls instead of end. #509

CedNaru opened this issue Oct 12, 2023 · 2 comments
Assignees
Labels
bug Something isn't working enhancement New feature or request

Comments

@CedNaru
Copy link
Member

CedNaru commented Oct 12, 2023

It's something that has been bothering me for a while, debugging the project and going back and forth between the Kotlin and C++. When, for some reason, the data in the shared buffer are not valid, the module is going to throw an error. The error is often things like the number of arguments not matching, or the type of the variable not being the expected one, etc....
The issue with that approach is that those errors are often thrown before rewind() is called, leaving the buffer cursor is an invalid position instead of going back to 0.
The consequence of that is that once you have got a first sharedbuffer error, it's going to cascade into all the next calls throwing errors as well because the cursor was never reset and so the data is incorrectly read. It makes the logs full of irrelevant messages, making it harder to decipher.

I propose to change all the TransferContext methods so rewind() is now called at the beginning of each call. It shouldn't alter the regular behavior of the ShareBuffer and would also allow the module to "recover" after an error. The only difference would be that the Sharedbuffer is always left at a non-zero position after being used. But I don't think there should be any issue, as the next method using the buffer should reset it anyway.

It's a simple change, only meant to make the module a bit easier to debug, by removing noises after a first error.

@CedNaru CedNaru added enhancement New feature or request bug Something isn't working labels Oct 13, 2023
@CedNaru CedNaru self-assigned this Oct 13, 2023
@chippmann
Copy link
Contributor

A much needed improvement indeed and a low hanging fruit. I see no issues with this approach.

@piiertho
Copy link
Member

Definitely a good idea. This will help a lot debugging.

@CedNaru CedNaru added this to the 0.7.1 milestone Oct 13, 2023
@CedNaru CedNaru closed this as completed Oct 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants