arkhipenko / TaskScheduler

Cooperative multitasking for Arduino, ESPx, STM32, nRF and other microcontrollers
http://playground.arduino.cc/Code/TaskScheduler
BSD 3-Clause "New" or "Revised" License
1.32k stars 243 forks source link

enable dynamicaly created Task to be self-destructable on last iteration #146

Closed vortigont closed 2 years ago

vortigont commented 2 years ago

Allow to dynamically create Task objects that could be self-destructed by Scheduler on last iteration. Mostly useful for one-shot tasks for postponed activities. It helps to save some memory and there is no need to keep track of expired task objects.

arkhipenko commented 2 years ago

I really like the idea. I will need to think through the implementation details though. Thank you for the idea and the effort!

arkhipenko commented 2 years ago

Implemented as version 3.7.0 An example is here: https://github.com/arkhipenko/TaskScheduler/tree/testing/examples/Scheduler_example19_Dynamic_Tasks_SelfDestruct

Enjoy and thank you for the idea.

vortigont commented 2 years ago

thanks @arkhipenko, looks good to me! Just if you wanna know my feedback: IMO Task::setSelfDestruct() method should be considered very dangerous in scenarios with self-destructing obects. I.e. I create an object and keep reference to it. But the scope keeping the reference cant guarantee it's validity at any random point in time. So, using any methods on an object reference that could possibly not exist any more is unsafe and may lead to assertion failure or use-after-free. On the other hand, methods like Task::reset() could introduce even more ambiguity. Sooner or later somebody will shot his own leg using this :) So my original idea was to make it like "fire-and-forget", once self-destructing object has been created, there is no way of turning back and any references to it should be considered as invalid. As a compromise Task::setSelfDestruct() could be a private method, but Task::reset() still could lead to a mem leak. That was just my thoughts in general. Thanks for the implementation anyway! I used to use an ugly garbage collector before this. :)

arkhipenko commented 2 years ago

@vortigont - I absolutely agree. But it is a problem with the entire library, there are other ways to be careless and screw things up. I just hate to create functionality you cannot control. I will add a WARNING to the Wiki (if anyone reads it.. :) )

arkhipenko commented 2 years ago

but Task::reset() still could lead to a mem leak.

By the way - why do you think reset could introduce a memory leak? It does not allocate anything. All I see is that it could break the execution chain - something I should probably code around.

Here https://github.com/arkhipenko/TaskScheduler/blob/e7703d441b8fd6cd481853dc594b24edd55dfc15/src/TaskScheduler.h#L522 If task is part of the chain, this unsafely yanks it out. If there were any tasks after it, they will not be executed. A safer way is to call Scheduler::deleteTask here if iScheduler is not NULL. But even then, the double-linked list is fragile, no question.

arkhipenko commented 2 years ago

Ha! Task::reset() is actually a private member (unless you use _TASK_DEBUG to make all methods public, which is not recommended). We should be safe!

vortigont commented 2 years ago

Ha! Task::reset() is actually a private member

Huh! Indeed :) I missed it. I was thinking about calling reset and loosing pointer later somehow. With a private method it should be OK then. :)

If task is part of the chain, this unsafely yanks it out. If there were any tasks after it, they will not be executed. A safer way is to call Scheduler::deleteTask here if iScheduler is not NULL.

Yeah, this could potentially be a weak spot. In one of my projects I've used a linked list of std::shared_ptr's and it worked automagically - once you kick the object off a list it will self-destruct :) But you need a list as a container, your tasks a linked to each other - quite an interesting solution, btw. My first idea was that a Scheduler class is a container, but to my surprise I found that it was an iterator actually :)

arkhipenko commented 2 years ago

Tasks call Scheduler::deleteTask() in the destructor, so should be safe from the chain perspective. And yes, since original TS was created for things like Uno and Nano, every byte counted, hence compile options and Scheduler as an iterator rather than container. Size and efficiency were (and still are) the main goals