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

Add fn to find out if Host can be finished #1182

Merged
merged 3 commits into from
Nov 8, 2023

Conversation

leighmcculloch
Copy link
Member

@leighmcculloch leighmcculloch commented Nov 8, 2023

What

Add fn to find out if Host can be finished.

Why

So that the SDK can ask the Host if it has no more references left and it is able to be finished without calling try_finish which is destructive.

I plan to use this in:

@leighmcculloch leighmcculloch marked this pull request as ready for review November 8, 2023 16:14
@dmkozh
Copy link
Contributor

dmkozh commented Nov 8, 2023

I'm not necessarily opposed to this change, but I wonder if it's needed for your purpose. You can get read-only access to storage and events at any point - there is really no need to destruct host to do that (and IIUC the linked PR doesn't actually call try_finish). try_finish is there mostly as a minor optimization for e2e flows and it really just ensures that there is no bug that causes something to hold a reference.

@leighmcculloch
Copy link
Member Author

I'm not necessarily opposed to this change, but I wonder if it's needed for your purpose.

The SDK needs to know if the Host has a strong ref count greater than 1 because it is the only way the SDK can tell if the Env being dropped is the last holder of the Host. Env's get dropped all the time, but they all hold a shared ref to the Host via the internal Rc

I could add a ref_count fn that simply returns that. But given the Host has a concept of being "finishable" that is directly tied to whether the host is a unique ref or not (a ref count of 1), it feels like it works to add that function based on that terminology.

@dmkozh
Copy link
Contributor

dmkozh commented Nov 8, 2023

I see, thanks for the explanation.

@leighmcculloch leighmcculloch added this pull request to the merge queue Nov 8, 2023
Merged via the queue into main with commit 6d26ca6 Nov 8, 2023
10 checks passed
@leighmcculloch leighmcculloch deleted the add-ref-count-to-host branch November 8, 2023 17:52
github-merge-queue bot pushed a commit to stellar/rs-soroban-sdk that referenced this pull request Nov 11, 2023
### What
Autosave a test snapshot file on every test exit.

### Why
To encourage the over-time consistency testing of contracts. To provide
a mechanism where any developer who commits the generated files can see
changes in observable behavior that arises unexpectedly over time.

Close #1082

### Merging
To be merged to `main` after:

- [x] Adding events to the `Snapshot`.
- [ ] ~Adding budget to the `Snapshot`. _(Maybe, seems less clear if
should be included.)_~
- [x] Support multiple Envs in a single test writing multiple files.
- [x] #1142
- [x] stellar/rs-soroban-env#1182
- [x] #1135
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