search

ETLCPP / etl

Embedded Template Library
https://www.etlcpp.com
MIT License
2.25k stars 392 forks source link

Bug: circular reference in callback_timer_* when start() is called inside callback for repeating timers #967

Open ronaldd2 opened 1 month ago

ronaldd2 commented 1 month ago

The callback_timer_locked, callback_timer_atomic and callback_timer_interrupt is restarting the timer automatically after the callback is handled for repeating timers. If inside the callback the period is changed the timer is stopped, so normally you should call a start timer to start it again. When doing this from within in the callback, the timer is inserted twice in the list and a circular reference is created in the linked list.

The superseded callback_timer doesn't have this problem as there the timer is inserted before the callback is called. I see this behavior is changed 2 years ago in the other versions. Can you remember what the rationale for this change was?

Maybe changing the period inside the callback for a repeating timer is not intended, but this is not documented. Also it's not possible to stop a timer in the callback as it will be started automatically after the call back.

The easiest fix, I think, is to skip the automatic insert if the timer.is_active() is set, as normally it not possible to do an insert twice. Moving the insert back is not a real option I think as this gives totally different behavior for existing code.

Thanks in advance

KammutierSpule commented 1 month ago

Hi @ronaldd2 would you like to have a look to see if this is related or duplicated of https://github.com/ETLCPP/etl/issues/954

ronaldd2 commented 1 month ago

There is some relation, the unit test can indeed trigger this exact flow as the unit test is starting it's own timer in the callback.

KammutierSpule commented 1 month ago

There is also this one:

https://github.com/ETLCPP/etl/issues/952

because "If inside the callback the period is changed the timer is" changed, and you are using a mutex protected timer, then you will get a deadlock.

itavero commented 2 weeks ago

The superseded callback_timer doesn't have this problem as there the timer is inserted before the callback is called. I see this behavior is changed 2 years ago in the other versions. Can you remember what the rationale for this change was?

@jwellbelove Can you perhaps shed a light on this?

jwellbelove commented 2 weeks ago

I'll take a look as soon as I can.