dop251 / goja_nodejs

Nodejs compatibility library for Goja
MIT License
336 stars 81 forks source link

Fix deadlock on calling `EventLoop.Stop` #38

Closed zyxkad closed 2 years ago

zyxkad commented 2 years ago

If we calling EventLoop.Stop on a stopped loop, it will hang.

Calling Stop() on an already stopped loop or inside the loop will hang.

And if all goroutines are hanging:

fatal error: all goroutines are asleep - deadlock!

I fixed EventLoop.Stop on call on a stopped loop, it will never hang now.

dop251 commented 2 years ago

Why do you need it? The idea is that Start and Stop should not be called concurrently and therefore the caller (being a single goroutine) should be aware of the current state of the loop.

Besides, your solution does not fully prevent deadlocks, they may still occur if Stop is called concurrently.

zyxkad commented 2 years ago

Why do you need it? The idea is that Start and Stop should not be called concurrently and therefore the caller (being a single goroutine) should be aware of the current state of the loop.

Besides, your solution does not fully prevent deadlocks, they may still occur if Stop is called concurrently.

I want to solved the panic when you do EventLoop.Run in the other goroutine and do EventLoop.Stop to stop the loop

dop251 commented 2 years ago

You can't simply use Run() and Stop().

If it is implemented without the deadlock (which is easy enough to do, just by changing the setting of canRun to false to an aux job), there is a fundamental synchronisation problem: if the Stop() finds the loop not running it can't know whether it's because it's already stopped or not yet started (because setRunning() has not been called yet). In this case there are 2 possible scenarios: it can just do nothing or it can set a flag so that the subsequently started run loop immediately stops (or not starts at all).

The problem with the first approach is that if Stop() is called too soon it will be ineffective because the run loop will start afterwards as normal and won't stop. The second approach means if the loop was not running and you try to use it again it will stop immediately after you call Run() or Start().

That's why currently the only allowed way is to use Stop() with Start() and to make sure they are never called concurrently.

I'm not sure what is your use case but I could change Stop() so that it uses an aux job rather than normal job (which would make sure it doesn't hang and allow calling it from the loop) and make it a no-op if the loop is not running. The rest can be sorted out using external synchronisation.

zyxkad commented 2 years ago

If it is implemented without the deadlock (which is easy enough to do, just by changing the setting of canRun to false to an aux job), there is a fundamental synchronisation problem: if the Stop() finds the loop not running it can't know whether it's because it's already stopped or not yet started (because setRunning() has not been called yet). In this case there are 2 possible scenarios: it can just do nothing or it can set a flag so that the subsequently started run loop immediately stops (or not starts at all).

I support your idea that Start can't call with Stop at the same time. It's an unstable operation anyway. Users should avoid this operation.

zyxkad commented 2 years ago

The problem with the first approach is that if Stop() is called too soon it will be ineffective

It should be ineffective, I don't think it's a problem

The second approach means if the loop was not running and you try to use it again it will stop immediately after you call Run() or Start().

I see that's your solution, but I don't think it's a good idea. Who will call Stop first and then Start or Run