Re: Termination of periodic tasks

[Natasha Kerensikova <lithiumcat@instinctive.eu>]

On 2014-06-17, Jacob Sparre Andersen <jacob@jacob-sparre.dk> wrote:
> Natasha Kerensikova wrote:
>
>> So let's consider instead real code that actually (more or less)
>> works:
>> https://github.com/faelys/natools/blob/trunk/src/natools-cron.ads
>> https://github.com/faelys/natools/blob/trunk/src/natools-cron.adb (I
>> whipped it together yesterday after thinking about Dmitry's
>> suggestion, but it's still only a rough draft, there are issues to
>> iron out before using it for real, like handling map key
>> collisions. However all comments are still welcome.)
>
> I've posted a pull request which eliminates all explicit memory
> management from the package. :-)

I was hesitating between posting my comments here or on the pull
request. I decided to post them here so that others can offer their
views and their explanations on the points I raise. If that's not the
best choice, please tell me so and I won't do it again.

> ... But not solved your actual problem. :-(

Basically that's my main concern: you turned a partial solution (that
works fine as long as global objects are explicitly reset) into a
complete non-solution (that never terminates). I'm afraid I'll have to
reject the pull request for this reasons.

But still, your version intrigues me. What go to such great lengths
only to eliminate memory management?

Maybe I'm too scarred by having forged myself into a C programmer that
can code for years without committing any resource leak, but I have
trouble to see what is so wrong with memory management to want so much
to eliminate it.


I understand that the less resource management the better is a good rule
of thumb, just like the less code the better. But isn't it a relatively
mild benefit to balance against all costs?

The core idea around which the package has been built, is to have a
worker task that terminate whenever the job map is empty, hoping it
would happen at program termination. And since it can also happen
temporarily during the program execution, there needs a way to
"unterminate" it when the job map is no longer empty. Since as far as I
know there is no way to restart a terminated task, reusing the task
object. So the only solution is to deallocate the terminate task object
and allocate a new one, ready to start.

My understanding of "explicit memory allocation" is stuff that involves
"new" keyword and Ada.Unchecked_Deallocation instances, and the
task-restart trick is the only explicit memory allocation I see in the
original Natools.Cron.

Getting rid of it means either an interminable static task like your
proposal, or hiding the allocation/deallocation behind a container or
some other wrapper, that seem of little value in that situation.


However, your proposal also eliminates the uses of Ada.Finalization and
of Natools.References. Even though I don't consider them as explicit
memory management, it seems you do. To me, Natools.Reference is as much
of an abstraction as Indefinite_Ordered_Maps, so I don't see any
explictness being remove through the use of the latter instead of the
former. And Ada.Finalization is about any resource management, and I
would say less for memory than for other resources. Anyway, it feels
like an extremely mild mechanism, I can't see why one would want to
avoid it.


Moreover, by removing Natools.References, you no longer have reference
semantics, so you cannot move references from one part of the code to
another, for example from the map inside the protected object to the
task.

I would guess that's why you moved the callback execution from the task
body to a procedure in the protected object. However, this has serious
drawbacks:

  * First, it becomes a bounded error to perform any potentially
blocking operation, for example Text_IO.Put_Line which I used in my
little example to test the package while developing it.

  * Second, it locks the database while running the callback, so the
callback cannot change its own period or unregister itself from the
service.

Also, not having references means copying the object around, which you
also do (but could have avoided using Update_Element), so you can no
longer store any mutable internal state in the callback object. My
little test example had a counter in it, and that is no longer possible.


Lastly, you conflated the callback interface with the job
identification, but probably realized that there can still be several
copies of the same objects, so you had to rely on a new Event_ID type.

However the Event_ID are ever-increasing values of Positive, without any
overflow management. One could probably hope that no sane
implementation-defined limit would be reached in this package, but even
though it's a fair assumption I would rather not assume it if that has
no cost.

Or is it only to solve the key collision issue? In that case I had
something else in mind (I will probably commit it tomorrow, no that I
have committed a test case for the issue).



So in summary, you did indeed eliminate explicit and
not-so-explicit-to-me memory management, but at the cost of:
  - breaking the partial solution to the problem considered,
  - forbidding potentially blocking operations in the callbacks,
  - forbidding callback management from within callbacks,
  - making it harder to write callbacks that inherit from
    Ada.Finalization.* (e.g. because they hold resources like files)
    or anything else,
  - introducing software aging with an implementation-defined threshold.

Is it really worth it? Is there a lesson here I'm failing to grasp?


Thanks for your comments,
Natasha