DelSkayn / rquickjs

High level bindings to the quickjs javascript engine
MIT License
504 stars 63 forks source link

Unsound: Race condition for `Context` with parallel enabled. #151

Closed DelSkayn closed 1 year ago

DelSkayn commented 1 year ago

The Context type in rquickjs implements Send and Sync when the parallel feature is enabled.

However Context uses non-atomic reference counting internally. This means that if a Context is send across a thread a race condition can happen when two Context objects are simultaneously cloned or dropped and the reference count is raised or lowered.

There are two options to solve this:

I am currently leaning towards the first solution

DelSkayn commented 1 year ago

I also found that Persistant implements Send when parallel is enabled.

This is also unsound. Persistant use non-atomic reference counting which again can cause race conditions when send across threads.

katyo commented 1 year ago

Maybe the best solution is implementing atomic rc in quickjs itself.

DelSkayn commented 1 year ago

I have looked into trying to implement atomic rc in QuickJS but it seems rather hard to do. Reference counts in QuickJS are not like Arc as they also keep track of cyclic references and have a GC cycle which does a lot of subtracting and adding of reference counts. Since it isn't build with atomic reference counts in mind I am guessing the current implementation won't be to happy if a reference count changes cause of a value being dropped in another thread.

If we where to implement atomic reference count we would probably need to rewrite the garbage collector.

I think, at least for now, I will just wrap Context in an Arc whenever the parallel feature is enabled and make Persistent not Send even if parallel is enabled.

DelSkayn commented 1 year ago

I have wrapped Context in Arc's when parallel is enabled and I have added an annoying workaround with a channels for dropping Context when the runtime is locked with async runtimes. Since async mutexes can't block in a destructor I had to send contexts over a channel to be dropped once the lock is released. It works but it is pretty annoying.

With these changes this issue should now be fixed.