Support reference / unreference of watchers about event-loop HOT 12 OPEN

@clue We came across a situation where the ability to unreferenced watchers would be very helpful.

We're writing a metric library where we are periodically sending data. Since we do not want to change the behavior of the script, we need to be able to un-reference the timer, so that when all normal timers have completed (everything but our metrics timer), the loop will exit.

from event-loop.

Thanks for raising this, this is an interesting feature! Many people (including myself) have been using ReactPHP in production for years and while I can see some potential use cases for this, I wonder why nobody seems to bother after all this time. It's currently unclear to me how much value it actually provides and I would like you to keep in mind that (while reasonable) it stills adds a fair bit of complexity.

Given that its value is currently unclear, I don't see this feature on the current roadmap any time soon. I suppose we would be super happy to accept PRs and/or discuss this further though 👍

It seems like you have some more experience with using this feature, perhaps you can link to some discussion about the pros of this approach? Thank you!

from event-loop.

Use case is mainly GC watchers or timeout watchers that are not of active interest. Sometimes it's useful to have the event loop just exit once there's nothing interesting anymore. It's probably a smaller usage base than long running applications that never exit.

Discussion: async-interop/event-loop#12

from event-loop.

I'm still trying to understand the motivation for this feature. Unless I'm missing something it looks like the link you've posted discussed more the API of how to expose this functionality and less so why exposing it in the first place.

I'm curious what "not of active interest" actually is. ReactPHP's timers currently work around the idea that you do care about them being executed and that the loop should not be stopped while timers are still pending.

I've personally used ReactPHP in plenty of projects, long-running and short-lived ones alike and have yet to come up with a good reason for "unreferenced watchers". I'm not saying there's no value in them, I'm just curious to find what value they provide over the current approach 👍

from event-loop.

I'm still trying to understand the motivation for this feature. Unless I'm missing something it looks like the link you've posted discussed more the API of how to expose this functionality and less so why exposing it in the first place.

I know, it's a lot about how and not why, but it also covers why a bit. I can't link any other discussion right now.

I'm curious what "not of active interest" actually is.

• stat cache garbage collection
• another cache invalidation watcher
• exit code watcher for processes
• signal watchers

These are all things that run in the background, but are not of active interest. Maybe there hasn't been a need yet, because there is way less keep-alive functionality in ReactPHP's libraries? I don't know.

I just opened the issue as a possible future extension, if BC has to be broken anyway because of #104 for example.

from event-loop.

Thanks for these interesting links!

• stat cache garbage collection: Not used by ReactPHP and I don't really see this happen any time soon. Running this in an unpredictable timer is unlikely to happen in ReactPHP. I think running this explicitly on demand makes more sense (YMMV).
• another cache invalidation watcher: Not used by ReactPHP and there are currently plans to handle this through the cache API and not an external timer. Handling this as part of the API adds some minor complexity but makes this more reliable and accurate than relying on an external trigger. (reactphp/cache#11)
• exit code watcher for processes: Not used by ReactPHP as it starts a (what you would call "referenced") timer to make sure processes always end with an exit event.
• signal watchers: Not used by ReactPHP and there are currently plans to watch for pending signals as part of the loop iteration. (#104)

I think it's obvious that ReactPHP and AMPHP took different design choices here and I don't think that either is necessarily better than the other. That being said and while I appreciate the discussion, I'm still not sold on the idea that adding this complexity to the event loop is worth it 👍

from event-loop.

@kelunik I still think you're making a valid point here.

The current behavior has been implemented >5 years ago which means that this is how this component has always worked since its inception. I'm not suggesting it's perfect, but it has clearly stood the test of time and has been used in production for years without any major issues.

I'd like to emphasize that I think your suggest changes may make sense from a standards body perspective – however, they require a major BC break and it's currently unclear how much would be gained from a user perspective by introducing this.

We currently try to focus our efforts on stabilizing things and would like to tag a stable release of this component in the upcoming weeks. Addressing this issue in time would require a substantial effort from our side – with unknown practical gain for our users on the other side.

This basically means that if you or anybody else feels like picking this up and addressing this in time for the next stable release, then I'm all for getting this in! As such, PRs are highly welcome! Thank you!

from event-loop.

Note that this is supported by Node timers as well as in UV. Unreferencing watchers/timers is not some strange thing invented by Amp. Rather, it's unusual that it's not supported in React.

Personally, I have implemented many repeat timers in the application layer which I would 'unreference' if the option existed.

from event-loop.

@davidwdan I think @clue is right and this could potentially be a BC break. For me it finally ticked when @davidwdan when mentioned a few days ago and again today signals are awesome but there is no need for them to let the loop run.

So after the UV loop is in I wanne experiment with this and see if I can work out a way for it not to be a BC break. No promises though 😄

from event-loop.

@WyriHaximus Well, it has to be a BC break, because a change to the interface is required?

from event-loop.

@kelunik I'll get back to you on that once I've put some time into this and have a prototype 😉

from event-loop.

I'm interested in implementing this. (Mostly because I have an use case I could use this myself).

For this I'd like to introduce two methods on the LoopInterface, which handles dereferencing and referencing of streams and timers - deref and ref respectively (or rather full name?).

The question would now be if the TimerInterface should be extended as well. But since cancel was removed from it somewhile ago (and transitioned to cancelTimer on LoopInterface), makes me question whether this is actually a good idea. I'd say we just leave it at the LoopInterface methods, but I'd like some opinions from the maintainers.

Is there anything else I should look out for/implement for this feature?

edit:
@WyriHaximus said to create an extended loop interface to avoid BC in v1.x and then drop it for v2.0. Do the other maintainers agree to go with this?

from event-loop.