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

Pass ref to host to contract invocation hook #1230

Merged
merged 2 commits into from
Nov 18, 2023

Conversation

leighmcculloch
Copy link
Member

What

Pass ref to host to contract invocation hook.

Why

So that hook implementations have access to the host without needing to hold a reference to the host via some other means.

This is the pattern used on other hooks in the Host.

@dmkozh
Copy link
Contributor

dmkozh commented Nov 17, 2023

I've intentionally omitted host to keep things simple - we use the lifecycle hooks in different context and don't expose these. You probably would want to capture the whole env with this, so passing the host seems redundant.

@leighmcculloch
Copy link
Member Author

If the impl of the hook holds a ref to the Host, won't the Host will live forever and never get dropped because its strong count will never reach zero?

@dmkozh
Copy link
Contributor

dmkozh commented Nov 18, 2023

If the impl of the hook holds a ref to the Host, won't the Host will live forever and never get dropped because its strong count will never reach zero?

I'm not sure why should the hook hold a strong reference to the host (if you capture it by reference); also it seems like it would be possible to mess up even if the host reference is passed as an argument (if you're not careful with how the env is captured). But I haven't played much with closures and borrow checker, so I could be missing something here.
Anyway, if you feel like this is necessary, you also need to update the hook call-sites to pass host.

@leighmcculloch
Copy link
Member Author

Admittedly I didn't test it using my own auth ref captured by the hook impl, but I don't see how there wouldn't be a cyclic ref here.

Host holds the hook, then hook holds a copy of the host that it captured (cloned). The ref count won't reach 0, unless there's some mechanism to handle that in Rust.

@dmkozh
Copy link
Contributor

dmkozh commented Nov 18, 2023

Admittedly I didn't test it using my own auth ref captured by the hook impl, but I don't see how there wouldn't be a cyclic ref here.

I've been rather thinking about capturing the reference, but that simply won't compile. So I guess that's the way to go...

@leighmcculloch leighmcculloch marked this pull request as ready for review November 18, 2023 00:42
@leighmcculloch leighmcculloch added this pull request to the merge queue Nov 18, 2023
Merged via the queue into main with commit cfd2df9 Nov 18, 2023
9 of 10 checks passed
@leighmcculloch leighmcculloch deleted the underpresence-showance branch November 18, 2023 00:52
github-merge-queue bot pushed a commit that referenced this pull request Nov 18, 2023
### What
Trigger top contract invocation finish hook after storing the auth
manager.

### Why
So that hook implementations can access the auth state that occurred
during the run.

### Merging
Intended to be merged to `main` after:
- #1230
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