Privatize storage handling #141
Conversation
lib/vm/src/instance.rs
Outdated
| pub fn recycle(instance: Self) -> (wasmer_runtime_core::Instance, A) { | ||
| (instance.wasmer_instance, instance.api) | ||
| pub fn recycle(instance: Self) -> (wasmer_runtime_core::Instance, A, Option<S>) { | ||
| let storage = take_storage(instance.wasmer_instance.context()); |
There was a problem hiding this comment.
What about making this Option.
If storage is none we can just drop api. It is a simple, clone able struct, so little is lost
There was a problem hiding this comment.
I don't understand what you mean. The storage variable is of type Option<S> already due to the return type of take_storage from context.rs.
I was even thinking in the opposite direction: why is this optional? How can this ever be unavailable?
There was a problem hiding this comment.
2 points. Seems my message was eaten by the github
- I would make the second result Option < Extern > (seems the rust generic syntax got eaten as an html tag)
- If we now privatize all external ways of having no Storage set (take_storage), and can reason through all internal cases to prove to ourselves this will never happen (like during with_storage), then we can just return
Externinstead ofOption<Extern>and panic if not present (only if we prove to devs, not compiler, this will never happen).
I still stand with my point of returning Extern not Storage there
There was a problem hiding this comment.
-
makes sense, implemented.
-
I found a case where the instance has no storage anymore:
#[test]
fn has_no_storage_when_nested() {
// this should fail with the assertion, but not cause a double-free crash (issue #59)
let instance = mock_instance(&CONTRACT_0_7);
instance.with_storage(|storage1| {
instance.with_storage(|storage2| {
assert!(false, "storage missing, so this must not be executed");
});
});
}But for the purpose of this PR we can leave it as is I think.
webmaster128
force-pushed
the
privatize_storage-handling
branch
from
20db69e to
6781f3c
Compare
lib/vm/src/instance.rs
Outdated
| /// The components we want to preserve are returned, the rest is dropped. | ||
| pub fn recycle(instance: Self) -> (wasmer_runtime_core::Instance, A) { | ||
| (instance.wasmer_instance, instance.api) | ||
| pub fn recycle(instance: Self) -> (wasmer_runtime_core::Instance, A, Option<S>) { |
There was a problem hiding this comment.
Very nice, yes, consume Self
There was a problem hiding this comment.
Yeah, Self the type, not the reference. I tried to do the same as an instance method but that does not work trivially.
There was a problem hiding this comment.
This is good. It means it "consumes" the object, so no one can use it afterwards.
I think this means the destructor is called at the end, so any objects owned by instance at the end of the method are freed
| pub fn recycle(instance: Self) -> (wasmer_runtime_core::Instance, A, Option<S>) { | ||
| let storage = take_storage(instance.wasmer_instance.context()); | ||
| (instance.wasmer_instance, instance.api, storage) | ||
| pub fn recycle(instance: Self) -> (wasmer_runtime_core::Instance, Option<Extern<S, A>>) { |
There was a problem hiding this comment.
Thanks for the changes. I know this just moved the same code blob down one level, but I prefer this to keep the public api simpler.
| let storage = take_storage(instance.wasmer_instance.context()); | ||
| (instance.wasmer_instance, instance.api, storage) | ||
| pub fn recycle(instance: Self) -> (wasmer_runtime_core::Instance, Option<Extern<S, A>>) { | ||
| let ext = if let Some(storage) = take_storage(instance.wasmer_instance.context()) { |
There was a problem hiding this comment.
Note: this is totally valid. You can also use a match statement here. I'm not sure which is "better" here. I like let when there is one option, and match for 3 or more. For 2, personal choice
Based on #136As discussed in https://github.com/confio/cosmwasm/pull/140#discussion_r373392309