runtime is not safe

[nivkner]

when you call Runtime::run you use the following code

let rt_ptr: *mut Runtime = &mut self;
unsafe { RUNTIME = rt_ptr };

the code assumes that initialization of the runtime occurs only once and that the rest of the code interacting with it happen at f();

however, Runtime::run() can be called from any thread, so it is possible to spawn a thread, create a new runtime and call Runtime::run again, causing a data race.

to fix this you can use std::sync::atomic::AtomicBool, which is in the standard library, so no extra dependency are needed, like this:

static RUNTIME_INITIALIZED: AtomicBool = AtomicBool::new(false);
static mut RUNTIME: *mut Runtime = std::ptr::null_mut();

pub fn run(mut self, f: impl Fn()) {
    if RUNTIME_INITIALIZED
        .compare_and_swap(false, true, Ordering::SeqCst) {
        panic!("the runtime must only be initialized once");
    }
    let rt_ptr: *mut Runtime = &mut self;
    unsafe { RUNTIME = rt_ptr };

    let mut ticks = 0;

    f();
    /// ...
}

[nivkner]

looking at the code again it seems like no accesses are synchronized, some of which are mutable, so this could cause data-races if modules are used in other thread.

also, raw pointers are de-referenced without checking for null while module function could be called without using Runtime::run at all.

and the signature of Runtime::run is pub fn run(mut self, f: impl Fn()) so the runtime exists on the stack and will be de-allocated when run exits. but module functions can be called after run exits, at which point there will be a use-after-free.

i think the best solution to maintain the global nature of Runtime is to use std::thread_local which will initialize on first access, need not be synchronized, is safe and still only depends on the standard library.

[cfsamson]

Thanks for the suggestions. I know this part is "bad" of the exact reasons you point out. Now, I do write a little bit about why and the motivation for it in the book, but of course, the best thing is to not encourage bad practices if I can avoid it.

Regarding the modules, the key is that I only do this to make the code look more like javascript which is not easy since Runtime will always be implicitly initialized when using javascript whether it's the browser or Node.

Maybe panicing if Runtime is not initialized is a bit better than what I do here.

I didn't consider thread_local, that's actually pretty smart since it will solve the problem of anyone trying to use the runtime from different threads. Another option I thought about was using std::sync::Once which is what lazy_static uses. I'll have a look at this a bit more when I have some time.