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

fix(js): free native strings #238

Merged
merged 3 commits into from
Sep 5, 2023

Conversation

genaris
Copy link
Contributor

@genaris genaris commented Aug 31, 2023

Not completely sure if it's the right approach (especially in react-native) but here is a simple attempt to free strings automatically in JS wrappers before returning to the caller, as discussed in #236.

Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Copy link
Member

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

This doesn't include the RN implementation yet right?

@@ -723,6 +726,8 @@ export class NodeJSAnoncreds implements Anoncreds {
const returnValue = handleReturnPointer<{ data: Buffer; len: number }>(ret)
const output = new Uint8Array(byteBufferToBuffer(returnValue))

this.nativeAnoncreds.anoncreds_buffer_free(returnValue.data)
Copy link
Member

Choose a reason for hiding this comment

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

Is a buffer also copied and not a ref?

Copy link
Contributor Author

@genaris genaris Aug 31, 2023

Choose a reason for hiding this comment

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

I've just done an update there because when creating an Uint8Array from a Buffer (this case in line 727) simply a new view is created, meaning (if I understand correctly) that no data is copied.

So I think it is safer to do this buffer free right after doing the text decoding and outputting to a string (which effectively copies the data).

Copy link
Contributor

Choose a reason for hiding this comment

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

It would depend on how ffi-napi ref-napi handles this. It seems to me that they copy the data, but I haven't checked the code, yet.

Copy link
Contributor

@berendsliedrecht berendsliedrecht left a comment

Choose a reason for hiding this comment

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

LGTM. TypeScript 5.2.2 has been released so we can use the new using declaration to probably make it even better. This could even be done in the shared part of the library fixing it for Node.JS and React Native

Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
@genaris
Copy link
Contributor Author

genaris commented Aug 31, 2023

This doesn't include the RN implementation yet right?

I couldn't test it deeply but I've updated those template createReturnValue functions related to strings (char *) and buffers (ByteBuffer) to call these clean up functions, so the fix should work also for RN.

@genaris
Copy link
Contributor Author

genaris commented Aug 31, 2023

LGTM. TypeScript 5.2.2 has been released so we can use the new using declaration to probably make it even better. This could even be done in the shared part of the library fixing it for Node.JS and React Native

Absolutely! I was looking at the release notes and using seems really promising for our exact use case (here and in aries-askar wrapper as well). But having it available in TS does automatically mean that we can use it in Node.js and React Native? I've seen that the related proposal is at stage 3 (candidate). Does that mean that it will take some time until JS engines like V8 or Hermes adopt it officially?

@andrewwhitehead andrewwhitehead merged commit a1f038d into hyperledger:main Sep 5, 2023
25 checks passed
@genaris genaris deleted the fix/js-string-free branch October 15, 2023 01:14
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.

4 participants