Skip to content

Add ability to reset timers.#135

Closed
macournoyer wants to merge 1 commit intosocketry:mainfrom
macournoyer:reset-timer
Closed

Add ability to reset timers.#135
macournoyer wants to merge 1 commit intosocketry:mainfrom
macournoyer:reset-timer

Conversation

@macournoyer
Copy link

I need to be able to change the timeout during a Fiber.scheduler.with_timeout block.

Types of Changes

  • New feature.

Contribution

@macournoyer
Copy link
Author

Will add some tests if that proposal make sense.

@ioquatix
Copy link
Member

In principle, I understand the use case and agree a bit more flexibility here could be useful - we actually had this previously in the timers gem - but I decided to optimise the implementation and reduce flexibility since the most common use case doesn't adjust the timeout.

Unfortunately, the proposed change is insufficient - it is not that simple to modify the timer handles, as they are effectively immutable - they may get inserted into a heap. It is tricky to evict them from the heap (since we don't know where they are), so instead when we pop handles out of the heap, we check whether they were cancelled before executing them.

As such, changing an existing handle beyond cancellation is not supported.

So, how to implement the proposed feature without a big cost?

We could expand the implementation here, but then every single timer would pay the cost despite the flexibility not being used much.

Therefore, I don't think we should modify this code, and instead, if the timeout needs to be extended, we can do that by cancelling a handle and scheduling a new handle. Here is an implementation: socketry/async#386 - we only use this interface if the user asks for it - so the existing use cases don't have any extra object allocations or overheads.

@ioquatix ioquatix closed this Apr 30, 2025
@samuel-williams-shopify
Copy link
Contributor

See https://github.com/socketry/timers/blob/23bccc713a244a339500318616fae79d9922a78f/lib/timers/timer.rb#L34-L93 for the timers gem code - as you can see, it's significantly more complex than the current implementation of Handle. Yet, we can implement most of the functionality on top of Handle efficiently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants