Make Persistent Send

[Sytten]

Right now, if you try to use Persistent with the Parallel and futures flag (AsyncContext) you won't be able to do so since Persistent is not marked as Send.

I am pretty sure this is safe since the restore function will take a lock on the runtime implicitly.

I am also confused about the comment: https://github.com/DelSkayn/rquickjs/blob/304db5d8ebe6b42cfa9d9cf6d11ff7d355bdf2ac/core/src/persistent.rs#L165-L166 I don't understand how that is possible reading the code. Does that mean we should use a manual drop to make sure the Persistent is dropped before the runtime or it doesn't matter?

[richarddd]

I am pretty sure this is safe since the restore function will take a lock on the runtime implicitly.

Agree

I don't understand how that is possible reading the code. Does that mean we should use a manual drop to make sure the Persistent is dropped before the runtime or it doesn't matter?

If you forget to restore a saved JS value, QuickJS will crash/error since it keeps track of references. This is to prevent memory leaks. So even if you're not using the saved value, or clone it, you would need to restore it.

[Sytten]

If you forget to restore a saved JS value, QuickJS will crash/error since it keeps track of references. This is to prevent memory leaks. So even if you're not using the saved value, or clone it, you would need to restore it.

Are you sure about it? Like if I clone it outside of quickjs it can't know that I am holding more than one ref to it so if I restore them twice it should in theory not work by that logic.

I find the whole concept a bit unclear. Like if I am not using the pointer after the runtime is killed what does it matter that I still hold it? Just free the memory on runtime close and let me worry about it.

Maybe I could avoid using the persistent altogether, essentially I am loading some external code and I want to call a function on that code later on. I didnt find a way to get a module already loaded/eval by name.

[richarddd]

If you forget to restore a saved JS value, QuickJS will crash/error since it keeps track of references. This is to prevent memory leaks. So even if you're not using the saved value, or clone it, you would need to restore it.

Are you sure about it? Like if I clone it outside of quickjs it can't know that I am holding more than one ref to it so if I restore them twice it should in theory not work by that logic.

I find the whole concept a bit unclear. Like if I am not using the pointer after the runtime is killed what does it matter that I still hold it? Just free the memory on runtime close and let me worry about it.

Maybe I could avoid using the persistent altogether, essentially I am loading some external code and I want to call a function on that code later on. I didnt find a way to get a module already loaded/eval by name.

The GC in QuickJS will fail and crash, there is an assert checking reference count. If its not zero when engine is droped you will have a panic.

[Sytten]

Ok ok, I am trying to make it crash but it does seem pretty resistant. IDK if they changed something.

[Sytten]

I improved the safety message, it is true that "generally" the Persistent values cause a process exit but not always. Module for example will not but a Value will. Essentially if the struct doesn't have a Drop implemented then it won't crash the runtime. Also a restore is not required, a drop is sufficient.

[DelSkayn]

The problem with adding Send + Sync to Persistant is what happens on a drop?

If the Persistant contains a value with a ref-count, it will have it's ref-count decremented and possibly be dropped, without locking the runtime. If the runtime is meanwhile used by another thread you have a race-condition.

Thinking about it now, Persistant might have some soundness issues. You might be able to create unsound behavior by doing something with the runtime in the destructor of a value and then drop that value while the runtime is locked.

One solution is to lock the runtime in the destructor of Persistant, but I would like to avoid that as locking in a destructor is asking for dead-locks, also it doesn't work for the async runtime.

I introduced a channel into the async version of the runtime a while ago which dealt with the problem of how the drop a Context when the runtime was already locked. A Context which can't acquire a lock on the runtime while dropped will instead add itself to a channel which will be emptied before releasing a lock. Maybe we can do the same for Persistant.

[Sytten]

Yeah I agree this is not an easy problem, harder than I thought. The problem is is that the return value for the async runtime must be send so it makes it impossible to use with persistent at the moment. Personally I only needed to keep a module reference to be able to call a method on it later (so AFAIK it is alway safe) but yeah it is not always the case (I think we should have another to this usecase FYI, maybe a way to get a module by name if it is already loaded).

A channel is usually the way to go for sure, I have seen this pattern multiple time to get around the async drop problem. The only problem usually is you need something to read from that channel, I like the "read before dropping the lock" though there is a possibility that an object is not recycled immediately with this solution (if the lock is released at the same time the object is sent on the channel) but its probably fine.

Really the only alternative would be to spawn a worker thread for the engine and communicate with this worker thread via channels. This is essentially what https://github.com/HiRoFa/quickjs_es_runtime does AFAIK. But thats a big change in the lib structure.

[Sytten]

@DelSkayn Other thing to consider, it would be good to derive Trace for Persistent to avoid loops.