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

Borrow API v1.0 Proposal #44

Closed
wants to merge 4 commits into from
Closed

Borrow API v1.0 Proposal #44

wants to merge 4 commits into from

Conversation

kjvalencik
Copy link
Member

@kjvalencik kjvalencik commented Jul 20, 2021

Neon should provide APIs for safely and ergonomically borrowing binary data. Borrowing should leverage the type system for statically checking safety as much as possible without sacrificing flexibility.

Rendered

Implementation: neon-bindings/neon#780
Pre-release published in 0.10.0-alpha.1.

@dherman
Copy link
Contributor

dherman commented Jul 23, 2021

Does Node-API have an API for detaching the backing buffer out of an ArrayBuffer? That could be another future extension.

@dherman
Copy link
Contributor

dherman commented Jul 23, 2021

Seems like another drawback is you can’t mutably borrow multiple buffers (with eg a dynamic check they don’t alias or overlap). I don’t know if this is a rare enough use case not to matter.

@dherman
Copy link
Contributor

dherman commented Jul 23, 2021

@kjvalencik Remind me, does changing the scoped methods to &mut still allow multiple levels of nested scoped computations? I think there’s no reason that shouldn’t work, right?

@kjvalencik
Copy link
Member Author

kjvalencik commented Jul 23, 2021

Does Node-API have an API for detaching the backing buffer out of an ArrayBuffer? That could be another future extension.

Yes, it does with the following constraints:

  • It must not have already been detached
  • It must be externally allocated

Unfortunately, this wouldn't work with our current API design. We accept anything that is AsMut<[u8]> when creating external ArrayBuffer. Since this could be anything, it's important that we run the Drop. However, it's not possible to get the data pointer back out of an external buffer. We would need to have that feature added to Node-API or change Neon to only accept a certain type (e.g., Vec<u8>).

Seems like another drawback is you can’t mutably borrow multiple buffers (with eg a dynamic check they don’t alias or overlap). I don’t know if this is a rare enough use case not to matter.

It's a drawback of the "simple API" and the reason that the proposal includes a dynamically checked part. It's covered in this section: https://github.com/neon-bindings/rfcs/pull/44/files#diff-f7efd300745a6d0dcd2273fa31a292295151b581fb63b5ae24ef8661f1924520R71

@kjvalencik Remind me, does changing the scoped methods to &mut still allow multiple levels of nested scoped computations? I think there’s no reason that shouldn’t work, right?

Yes, it does because the scope creates a new Context and then you can create another nested scope off of that.

Copy link
Contributor

@dherman dherman left a comment

Choose a reason for hiding this comment

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

This is a great RFC. I left a detailed comment with the results of the conversation we had today about naming and API organization.

text/0000-node-api-borrow.md Outdated Show resolved Hide resolved
text/0000-node-api-borrow.md Show resolved Hide resolved
@dherman
Copy link
Contributor

dherman commented Aug 26, 2021

This looks great. I'm happy putting it into Final Comment Period.

@kjvalencik kjvalencik added the final comment period last opportunity to discuss the proposed conclusion label Aug 26, 2021
Copy link
Contributor

@dherman dherman left a comment

Choose a reason for hiding this comment

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

🎉🎉🎉

This is a great ergonomic and safety improvement!

The slight incompatible change is a cost, but as we get close to 1.0 this is our last opportunity to make a small number of incompatible changes. We should try to mitigate that cost e.g. by cutting a pre-release and giving people a chance to try out the migration.

@kjvalencik
Copy link
Member Author

Merged in 8beeb16

@kjvalencik kjvalencik closed this Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final comment period last opportunity to discuss the proposed conclusion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants