Throwing from disable() with invalid watcher
trowski opened this issue · 2 comments
I've been running into timing issues causing Loop::disable() to throw due to a watcher being invalidated during the invocation of the watcher callback. Essentially, if the watcher callback calls Loop::disable() at the end of the callback after side-effects from the watcher callback have invoked Loop::cancel(), an exception is being thrown. While it is possible to use an additional flag to code around this problem, it seems unnecessary.
Some example code: https://github.com/amphp/socket/blob/amp_v2/lib/Server.php#L37-L51
Putting the call to Loop::cancel() in a defer watcher seems to solve the issue here. I tried the same in the Socket class within the same lib here. However, this does not appear to have solved the problem, as tests still occasionally result in throws due to invalid watchers.
I propose that InvalidWatcherException should not be thrown from disable() with cancelled watchers, as disabling a cancelled watcher does not change the expectation that further events will not be received on the watcher. enable() should continue throwing when a cancelled watcher is given.
I've observed this issue myself too. While not too hard to fix once reproduced, it's a ticking bomb…
Everytime it can happen that the garbage collector gets triggered and the watcher cancel()'led, right before I ought to disable(). I only can find the obvious ones, but such scenarios where the cancel() gets triggered by a destructor through the cyclic garbage collector are not predictable and usefully testable.
If we don't change this as proposed, I'm looking forward to random crashes of long-running applications which are hard to debug (why did this happen???) ...
In the earlier amp implementations we just silently ignored disable() or cancel() calls for nonexistent (read: already cancelled) watchers ... The result is the same either way. I'm fine with this behavior, personally. Thoughts?