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

Refactor Resources #617

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Refactor Resources #617

wants to merge 3 commits into from

Conversation

filmor
Copy link
Member

@filmor filmor commented Jun 3, 2024 •
edited
Loading

  • Move storage of "Type->NIF Resource Type Handle" to a OnceLock map to allow implementing resource types without resorting to dynamic trait implementations
  • Implement resource types as a derive macro (this one is removed again)
  • Add direct access methods to get an immutable reference to the wrapped objects
  • Add corresponding converters
  • Add explicit resource registration functions
  • Add an additional optional "destructor" function to implement use cases like Lifetime issues when I want a resource to hold a NIF term #626, where a message is supposed to be sent when finalising a resource object
  • Implement monitoring support

This allows us to eventually deprecate the existing resource! macro and enables a path that fixes #606.

@filmor filmor mentioned this pull request Jun 3, 2024
Merged
3 tasks
@filmor filmor force-pushed the resource branch 3 times, most recently from ae70d92 to 5744d8f Compare June 8, 2024 11:03
@filmor filmor marked this pull request as ready for review June 8, 2024 11:04
@filmor filmor requested a review from a team June 8, 2024 11:04
@filmor filmor marked this pull request as draft June 8, 2024 12:41
@filmor
Copy link
Member Author

filmor commented Jun 8, 2024 •
edited
Loading

Another option, instead of a derive macro (or additional to one) would be to allow explicitly implementing a Resource trait and registering it using a rustler::resource!() outside of load, which would use the inventory::submit! under the hood. This would allow us to make the down callback for #376 (and dyncall and other potential callbacks) a trait function with an empty default implementation.

If we like the derive style, another alternative would be, to have a derive(Resource) and a derive(MonitorResource), where MonitorResource requires an implementation of a MonitorResource trait with a down_callback function and these would be different code paths with two sets of registrations. I think I'm in favour of that.

/edit: This is how it is implemented now: #[derive(MonitorResource)] + implementing the MonitorResource trait.

@filmor
Copy link
Member Author

filmor commented Jun 11, 2024

Note to self: #84 is also a very interesting approach (completely doing away with the necessary pre-registration for resource types!), I'm just not quite sure how to incorporate monitors into it.

@filmor filmor force-pushed the resource branch 2 times, most recently from dd69859 to f7aee7e Compare June 11, 2024 18:15
@filmor filmor mentioned this pull request Jun 15, 2024
Open
@filmor filmor mentioned this pull request Jun 28, 2024
Closed
@filmor
Refactor resources
e09db8c 
- Move storage of "Type->NIF Resource Type Handle" to a OnceLock map to
  allow implementing resource types without resorting to dynamic trait
  implementations
- Implement resource types as a `derive` macro
- Add direct access methods to get an immutable reference to the wrapped
  objects
- Add corresponding converters
@filmor
Add monitor resource implementation
542b9e7
@filmor
Refactor resources further
c4b94b2 
- Remove attempt of registration via macros in favour of explicit
  registration
- Require explicit trait implementation (not when using the existing
  macro for now)
- Allow implementing a "destructor" function that runs just before
  `Drop` but has access to the caller environment
@filmor filmor marked this pull request as ready for review July 1, 2024 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scrogson scrogson scrogson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

rustler::resource! produces compiler warning
2 participants
@filmor @scrogson