GC.KeepAlive

[martindevans]

As I understand it the GC.KeepAlive(store) calls have been added throughout the codebase wherever a store is converted into a StoreContext to ensure that the Store is not garbage collected while the StoreContext is in use. This feels like a dangerous footgun to leave lying around! If a single call to KeepAlive is missed it's unlikely to cause an immediate problem that would be caught in tests, instead it will result in difficult to reproduce bugs for users of the library.

Would adding a Store field to the StoreContext struct resolve the problem? We can always retrieve the Store through the weak handle, so we would do that eagerly in the constructor, instead of lazily in the property.

Of course the downside is that doing this would double the size of the StoreContext struct (8 bytes to 16 bytes). But it would allow us to get rid of the calls to GC.KeepAlive and completely eliminates the possible bug in all future features that use StoreContext.

[kpreisser]

Hi, that's a good thought. However, I think it is not enough to simply add a Store field to the StoreContext, as AFAIK it would be similar to declaring a local Store variable, and the compiler or GC could determine that the StoreContext.Store field is no longer accessed in the code, and make it eligible for GC while there is still a native call executing that takes a IntPtr context. You would still need to call GC.KeepAlive(context.Store) afterwards, to ensure that the Store is not GCed.

A possibility would be to have StoreContext implement a Dispose() method, which would call GC.KeepAlive(this.Store). That way you could use using var context = store.Context; to ensure the GC.KeepAlive() call always happens afterwards. However, this would have a (small) performance "penalty" due to the additional finally clause, and it would suffer from a similar issue: If you forget to use using ... and instead just write var context = Store.context;, then there is also no guarantee that the Store is not GCed prematurely.

(Note that this only applies to the unusual case where the user forgets to call Store.Dispose() or to use it with a using statement. When the Store is correctly disposed after using it, it is guaranteed that its handle (wasmtime_store_t*) is not deleted prematurely.)