undo-auto--undoable-change infloop

undo-auto--undoable-change infloop

[Michael Heerdegen]

Hello,

does somebody have a guess what is happening here?  After automatically
replacing thousands of matches in a buffer with my el-search, I got a
sluggish Emacs afterwards, after a short pause.  Emacs consumed 100% of
one CPU.  It took me hours to find something, and it seems to be
unrelated to my own code: `undo-auto--undoable-change'.  It seems to
recurse very deep, or to be called extremely often.  I managed to
produce a backtrace, see below.  How is that recursion even possible -
what could be going on?

TIA,

Michael


The backtrace (the first cursor blink afterwards seems to initiate the
problematic behavior):

  (timer-activate [t 25311 13221 560142 nil undo-auto--boundary-timer nil nil 540000 nil])
  (run-at-time 10 nil undo-auto--boundary-timer)
  (undo-auto--boundary-ensure-timer)
  (undo-auto--undoable-change)
  (timer-activate [t 25311 13220 972130 nil undo-auto--boundary-timer nil nil 860000 nil])
  (run-at-time 10 nil undo-auto--boundary-timer)
  (undo-auto--boundary-ensure-timer)
  (undo-auto--undoable-change)
  (timer-activate [t 25311 13220 562017 nil undo-auto--boundary-timer nil nil 682000 nil])
  (run-at-time 10 nil undo-auto--boundary-timer)
  (undo-auto--boundary-ensure-timer)
  (undo-auto--undoable-change)
  (timer-activate [t 25311 13220 175101 nil undo-auto--boundary-timer nil nil 375000 nil])
  (run-at-time 10 nil undo-auto--boundary-timer)
  (undo-auto--boundary-ensure-timer)
  (undo-auto--undoable-change)
  (timer-activate [t 25311 13219 728313 nil undo-auto--boundary-timer nil nil 962000 nil])
  (run-at-time 10 nil undo-auto--boundary-timer)
  (undo-auto--boundary-ensure-timer)
  (undo-auto--undoable-change)
  (timer-activate [t 25311 13219 407621 nil undo-auto--boundary-timer nil nil 486000 nil])
  (run-at-time 10 nil undo-auto--boundary-timer)
  (undo-auto--boundary-ensure-timer)
  (undo-auto--undoable-change)
  (timer-activate [t 25311 13219 71400 nil undo-auto--boundary-timer nil nil 128000 nil])
  (run-at-time 10 nil undo-auto--boundary-timer)
  (undo-auto--boundary-ensure-timer)
  (undo-auto--undoable-change)
  (timer-activate [t 25311 13218 786464 nil undo-auto--boundary-timer nil nil 766000 nil])
  (run-at-time 10 nil undo-auto--boundary-timer)
  (undo-auto--boundary-ensure-timer)
  (undo-auto--undoable-change)
  (timer-activate [t 25311 13218 551162 nil undo-auto--boundary-timer nil nil 891000 nil])
  (run-at-time 10 nil undo-auto--boundary-timer)
  (undo-auto--boundary-ensure-timer)
  (undo-auto--undoable-change)
  (timer-activate [t 25311 13218 258788 nil undo-auto--boundary-timer nil nil 94000 nil])
  (run-at-time 10 nil undo-auto--boundary-timer)
  (undo-auto--boundary-ensure-timer)
  (undo-auto--undoable-change)
  (timer-activate [t 25311 13218 89932 nil undo-auto--boundary-timer nil nil 31000 nil])
  (run-at-time 10 nil undo-auto--boundary-timer)
  (undo-auto--boundary-ensure-timer)
  (undo-auto--undoable-change)
  (timer-activate [t 25311 13217 955384 nil undo-auto--boundary-timer nil nil 756000 nil])
  (run-at-time 10 nil undo-auto--boundary-timer)
  (undo-auto--boundary-ensure-timer)
  (undo-auto--undoable-change)
  (timer-activate [t 25311 13217 847383 nil undo-auto--boundary-timer nil nil 663000 nil])
  (run-at-time 10 nil undo-auto--boundary-timer)
  (undo-auto--boundary-ensure-timer)
  (undo-auto--undoable-change)
  (timer-activate [t 25311 13217 765424 nil undo-auto--boundary-timer nil nil 571000 nil])
  (run-at-time 10 nil undo-auto--boundary-timer)
  (undo-auto--boundary-ensure-timer)
  (undo-auto--undoable-change)
  (timer-activate [t 25311 13207 807483 0.15 blink-cursor-timer-function nil nil 407999 nil])
  (run-at-time 0.15 0.15 blink-cursor-timer-function)
  (run-with-timer 0.15 0.15 blink-cursor-timer-function)
  (blink-cursor--start-timer)
  (blink-cursor-start)

When I redefine `undo-auto--undoable-change' with an empty body, the
problem disappears.  I could not reproduce something similar with normal
query-replace.

Re: undo-auto--undoable-change infloop

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Date: Tue, 26 Jul 2022 05:54:42 +0200
> 
> does somebody have a guess what is happening here?  After automatically
> replacing thousands of matches in a buffer with my el-search, I got a
> sluggish Emacs afterwards, after a short pause.  Emacs consumed 100% of
> one CPU.  It took me hours to find something, and it seems to be
> unrelated to my own code: `undo-auto--undoable-change'.  It seems to
> recurse very deep, or to be called extremely often.  I managed to
> produce a backtrace, see below.  How is that recursion even possible -
> what could be going on?

I don't know what happened, but undo-auto--boundary-timer is invoked
10 sec after the timer is set.  Perhaps this kind of problem can
happen when there are many undoable changes, and processing them takes
more than 10 sec?

Re: undo-auto--undoable-change infloop

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> I don't know what happened, but undo-auto--boundary-timer is invoked
> 10 sec after the timer is set.  Perhaps this kind of problem can
> happen when there are many undoable changes, and processing them takes
> more than 10 sec?

Maybe what I saw is only an artifact from debugging and tracing (which
also inserts into buffers...)

I'm totally unsure what is going on.  Is there anything I could try to
turn off?

A CPU profiler report looks like this, with stuff less than 3% removed:

       24220  41% - timer-event-handler
       21654  37%  - timer-activate
       19231  33%   - timer--activate
       17774  30%      timer--time-less-p
       19949  34% - ...
       19949  34%    Automatic GC
       14040  24% + command-execute  ;; actual command execution time
            [...] [...]

This is a very strange result, not saying anything to me.

Michael.

Re: undo-auto--undoable-change infloop

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

>        24220  41% - timer-event-handler
>        21654  37%  - timer-activate
>        19231  33%   - timer--activate
>        17774  30%      timer--time-less-p
>        19949  34% - ...
>        19949  34%    Automatic GC
>        14040  24% + command-execute  ;; actual command execution time
>             [...] [...]

Though, since redefining `undo-auto--undoable-change' has the effect of
solving the issue, I guess it must be related to undo.

Michael.

Re: undo-auto--undoable-change infloop

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: emacs-devel@gnu.org
> Date: Wed, 27 Jul 2022 05:52:46 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I don't know what happened, but undo-auto--boundary-timer is invoked
> > 10 sec after the timer is set.  Perhaps this kind of problem can
> > happen when there are many undoable changes, and processing them takes
> > more than 10 sec?
> 
> Maybe what I saw is only an artifact from debugging and tracing (which
> also inserts into buffers...)
> 
> I'm totally unsure what is going on.  Is there anything I could try to
> turn off?
> 
> A CPU profiler report looks like this, with stuff less than 3% removed:
> 
>        24220  41% - timer-event-handler
>        21654  37%  - timer-activate
>        19231  33%   - timer--activate
>        17774  30%      timer--time-less-p
>        19949  34% - ...
>        19949  34%    Automatic GC
>        14040  24% + command-execute  ;; actual command execution time
>             [...] [...]
> 
> This is a very strange result, not saying anything to me.

Why does it seem strange to you?

Re: undo-auto--undoable-change infloop

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: emacs-devel@gnu.org
> Date: Wed, 27 Jul 2022 06:56:38 +0200
> 
> Michael Heerdegen <michael_heerdegen@web.de> writes:
> 
> >        24220  41% - timer-event-handler
> >        21654  37%  - timer-activate
> >        19231  33%   - timer--activate
> >        17774  30%      timer--time-less-p
> >        19949  34% - ...
> >        19949  34%    Automatic GC
> >        14040  24% + command-execute  ;; actual command execution time
> >             [...] [...]
> 
> Though, since redefining `undo-auto--undoable-change' has the effect of
> solving the issue, I guess it must be related to undo.

At the risk of saying the obvious: you do know that undo runs a timer
to amalgamate undoable changes?

Re: undo-auto--undoable-change infloop

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> > A CPU profiler report looks like this, with stuff less than 3% removed:
> > 
> >        24220  41% - timer-event-handler
> >        21654  37%  - timer-activate
> >        19231  33%   - timer--activate
> >        17774  30%      timer--time-less-p
> >        19949  34% - ...
> >        19949  34%    Automatic GC
> >        14040  24% + command-execute  ;; actual command execution time
> >             [...] [...]
> > 
> > This is a very strange result, not saying anything to me.
>
> Why does it seem strange to you?

That there is no Lisp function there besides the timer stuff.  Undo
stuff like amalgamating is done in Lisp, so such function(s) should
appear there if they do work or initiate it.  And 30% time spent in
`timer--time-less-p' doesn't look sane.  Seems that function is called
extremely often.

Michael.

Re: undo-auto--undoable-change infloop

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Date: Wed, 27 Jul 2022 19:06:29 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > > A CPU profiler report looks like this, with stuff less than 3% removed:
> > > 
> > >        24220  41% - timer-event-handler
> > >        21654  37%  - timer-activate
> > >        19231  33%   - timer--activate
> > >        17774  30%      timer--time-less-p
> > >        19949  34% - ...
> > >        19949  34%    Automatic GC
> > >        14040  24% + command-execute  ;; actual command execution time
> > >             [...] [...]
> > > 
> > > This is a very strange result, not saying anything to me.
> >
> > Why does it seem strange to you?
> 
> That there is no Lisp function there besides the timer stuff.  Undo
> stuff like amalgamating is done in Lisp, so such function(s) should
> appear there if they do work or initiate it.  And 30% time spent in
> `timer--time-less-p' doesn't look sane.

It's 30% for timer--time-less-p _and_its_callees_, so you should look
deeper.

The 34% figure for GC also doesn't look good, btw.

Re: undo-auto--undoable-change infloop

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> It's 30% for timer--time-less-p _and_its_callees_, so you should look
> deeper.

I don't know how.  The trace is fully expanded, there are no recorded
Lisp callees.

> The 34% figure for GC also doesn't look good, btw.

That, that doesn't look good overall...

Michael.

Re: undo-auto--undoable-change infloop

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> [...] you do know that undo runs a timer to amalgamate undoable
> changes?

Yes, that was one of my theories.  But then setting
amalgamating-undo-limit to 1 should fix the problem, which isn't the
case.  And the amalgamating functions should also appear in the profiler
output, but there are none.

Michael.

Solved (was: undo-auto--undoable-change infloop)

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
> > [...] you do know that undo runs a timer to amalgamate undoable
> > changes?

But that put me on the right path, thank you very much.

I was using worker buffers.  These were filled, modified and erased
thousands of times in my test scenario.  Doing (setq buffer-undo-list t)
in these buffer solved the problem.  Not sure what Emacs was actually
doing all the time, but the huge gc time probably was spent in
collecting the fallout from the huge undo records.

Thanks for mentioning also the obvious things,

Michael.

Re: Solved (was: undo-auto--undoable-change infloop)

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: emacs-devel@gnu.org
> Date: Thu, 28 Jul 2022 05:10:52 +0200
> 
> Michael Heerdegen <michael_heerdegen@web.de> writes:
> 
> I was using worker buffers.  These were filled, modified and erased
> thousands of times in my test scenario.  Doing (setq buffer-undo-list t)
> in these buffer solved the problem.

Any reason why you didn't use temporary buffers for that?  AFAIR, undo
is disabled there automatically.

> Not sure what Emacs was actually doing all the time, but the huge gc
> time probably was spent in collecting the fallout from the huge undo
> records.

Yes, makes sense.

> Thanks for mentioning also the obvious things,

Glad I could be of help.

No, not solved (was: undo-auto--undoable-change infloop)

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> Any reason why you didn't use temporary buffers for that?  AFAIR, undo
> is disabled there automatically.

I actually do, I use buffers from evaluating

(list (generate-new-buffer " *El-Search Work Buffer A*" t)
      (generate-new-buffer " *El-Search Work Buffer B*" t))

But the problem is back.  After I had checked that `buffer-undo-list'
had been t all the time and my change could not have had an effect at
all, the problem instantly reappeared out of nothing.  I did not change
anything!  This is really mysterious and very frustrating.  I have
wasted hours now for this stuff and do not know a bit more than when it
started.

Michael.

Re: No, not solved

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> I have wasted hours now for this stuff and do not know a bit more than
> when it started.

Could be it is related to an older package I wrote some years ago.  Will
report back.

Michael.

Re: No, not solved

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Could be it is related to an older package I wrote some years ago.  Will
> report back.

Yes, seems "interaction-log.el" was the culprit.  I'm still not sure
why.  It uses a timer that fires every .1 seconds, but that performs
only trivial tasks that take only much much less time.

In my scenario, for some reason Emacs begins to choke, sometimes.
Making the time span a bit larger (e.g. .5 seconds) seems to help.

Michael.

Re: No, not solved (was: undo-auto--undoable-change infloop)

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: emacs-devel@gnu.org
> Date: Fri, 29 Jul 2022 02:40:11 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Any reason why you didn't use temporary buffers for that?  AFAIR, undo
> > is disabled there automatically.
> 
> I actually do, I use buffers from evaluating
> 
> (list (generate-new-buffer " *El-Search Work Buffer A*" t)
>       (generate-new-buffer " *El-Search Work Buffer B*" t))
> 
> But the problem is back.  After I had checked that `buffer-undo-list'
> had been t all the time and my change could not have had an effect at
> all, the problem instantly reappeared out of nothing.  I did not change
> anything!  This is really mysterious and very frustrating.  I have
> wasted hours now for this stuff and do not know a bit more than when it
> started.

So what is the problem, again?  Can you describe what your Lisp
program does, and what are the symptoms of the problem when it
happens?

If the problem is with timers, what does list-timers report when the
problem happens?

If you have a reproducible recipe, perhaps submit a bug report.

Re: undo-auto--undoable-change infloop

[Stefan Monnier]

>        24220  41% - timer-event-handler
>        21654  37%  - timer-activate
>        19231  33%   - timer--activate
>        17774  30%      timer--time-less-p
>        19949  34% - ...
>        19949  34%    Automatic GC
>        14040  24% + command-execute  ;; actual command execution time
>             [...] [...]

Maybe this is a symptom of having way too many timers?


        Stefan

Re: undo-auto--undoable-change infloop

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Maybe this is a symptom of having way too many timers?

Maybe.

Could it be that when executing the timer function takes longer than the
0.1 seconds repeat delta, the timer fires again?  I had hoped that I
would handle that by checking the idle time - but when the function is
run, Emacs is still idle (running timers counts as idle AFAIR), although
it is actually quite busy.

I once used an variable watcher on timer-list and saw that it contained
hundreds or thousands of instances of my timer.  But these were not
different instances, I only ever create one timer when stating logging.
Ant the value list looked like (#1=[ilog-timer] #1# #1# ...very very
many #1#s and a few different timers).

Now I'm trying to protect the timer using a flag with the purpose to
prevent the timer function to do anything when the timer function is
currently called.

But does that even make sense, can that happen at all?  Or could it be
that the waiting instances are processed sequentially but take longer to
process than 0.1 seconds - longer than new instances are added?

But that all seems very strange.  Maybe just something is not in
order...?

Michael.

Re: undo-auto--undoable-change infloop

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> I once used an variable watcher on timer-list and saw that it contained
> hundreds or thousands of instances of my timer.  But these were not
> different instances, I only ever create one timer when stating logging.
> Ant the value list looked like (#1=[ilog-timer] #1# #1# ...very very
> many #1#s and a few different timers).

BTW, all these copies appear out of nothing: the variable watcher saw
very few timers, and then suddenly that many the next time the
WATCH-FUNCTION was called.  If they would have been added explicitly
from Lisp, I would see intermediate steps AFAIU.

I collected a backtrace from the moment of time when the variable
watcher fired (via WATCH-FUNCTION).  It was totally unrelated and I'm
quite sure that it isn't related.

> Now I'm trying to protect the timer using a flag with the purpose to
> prevent the timer function to do anything when the timer function is
> currently called.
>
> But does that even make sense

After thinking about it, I think it doesn't make sense, and it also
doesn't work.

Michael.

Re: undo-auto--undoable-change infloop

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Sat, 30 Jul 2022 03:14:18 +0200
> 
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> 
> > Maybe this is a symptom of having way too many timers?
> 
> Maybe.
> 
> Could it be that when executing the timer function takes longer than the
> 0.1 seconds repeat delta, the timer fires again?

See timer-event-handler, which is the function invoked when a timer is
ripe, and handles both the invocation of the timer function and
rescheduling of the next timer invocation.  My reading of it is that
the above _could_ happen, but:

  . it can only happen if the timer function calls something that
    causes Emacs to wait for input, like sit-for, perhaps or
    accept-process-output, because otherwise Emacs will not check for
    ripe timers for as long as the timer function runs
  . the number of such "overlapping" invocations should not be more
    than 10, controlled by the value of timer-max-repeats

Re: undo-auto--undoable-change infloop

[Stefan Monnier]

> different instances, I only ever create one timer when stating logging.
> Ant the value list looked like (#1=[ilog-timer] #1# #1# ...very very
> many #1#s and a few different timers).

This is very curious indeed!
AFAIK there should not be `eq` elements in that list.


        Stefan

Re: undo-auto--undoable-change infloop

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> This is very curious indeed!
> AFAIK there should not be `eq` elements in that list.

I found something in a different place of my code.  That code uses
`timer-activate', but `timer-activate' doesn't check whether the timer
is already in the timer list, and that caused the duplicates I think.  I
didn't expect it to not check that.  Maybe that's the whole story
already.

Thanks for your help,

Michael.

Re: undo-auto--undoable-change infloop

[Stefan Monnier]

> I found something in a different place of my code.  That code uses
> `timer-activate', but `timer-activate' doesn't check whether the timer
> is already in the timer list, and that caused the duplicates I think.  I
> didn't expect it to not check that.  Maybe that's the whole story
> already.

I suspect that `timer-activate` was not intended to be used by
end-users, but in any case I added a check that signals an error if you
try to re-add a timer.
[ It would have been better to report this as a bug, so we'd have
  a bug-number to refer to.  ]


        Stefan

Re: undo-auto--undoable-change infloop

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> I suspect that `timer-activate` was not intended to be used by
> end-users, but in any case I added a check that signals an error if
> you try to re-add a timer.

Ok, thanks.  I intend to rewrite that stuff, I hope I end with some
cleaner implementation of the behavior I want.

> [ It would have been better to report this as a bug, so we'd have
>   a bug-number to refer to.  ]

Hmm, my questions often end like this, so I guess it's better to ask
them in the form of bug reports in the future ;-)

Thanks,

Michael.

Re: undo-auto--undoable-change infloop

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> I suspect that `timer-activate` was not intended to be used by
> end-users, but in any case I added a check that signals an error if
> you try to re-add a timer.

I now remember why I am using this.  The scenario is always the same, I
need this in quite a bunch of packages:

I want something happening automatically after certain user interaction
(e.g. scrolling, editing the minibuffer etc), say, running a certain
function F after a delay of DELTA seconds.  When the user does something
before DELTA seconds have passed, the time should count from full DELTA
again - the timer should be postponed.  If it's later, the timer should
be started anew (unless the "scenario" had been left).

In the code, I can't be certain whether the timer had been triggered (at
least without consulting "internal" information).  In any case the timer
still exists as a timerp fulfilling object (it's the value of some
variable I use).  So I used `timer-set-time' to update the preferred run
time and called `timer-activate' to ensure it's in the list (to cover
the second case: timer had been run).  And, as we had found out, that is
wrong when the timer is actually still in the list.

Is there a way to achieve what I want without relying on "internal data"
(like looking in `timer-list')?

Michael.

Re: undo-auto--undoable-change infloop

[Stefan Monnier]

> Is there a way to achieve what I want without relying on "internal data"
> (like looking in `timer-list')?

It's not 100% clear what is "internal" and what is not in `timer.el`, to
be honest, so I'm not completely sure what's the right answer.
But I think you can `cancel-timer` before `timer-activate`, which will
make sure it's not in the list any more.


        Stefan

Re: undo-auto--undoable-change infloop

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> But I think you can `cancel-timer` before `timer-activate`, which will
> make sure it's not in the list any more.

That solution also came to my mind.  Wastes a lot of time, though.  The
`timer-list' is traversed 3 times: First, to find the timer to delete
it, second to test if it's already present when trying to add it (your
change), third, to sort it in at the right position (though I don't
understand why that is necessary).  The list is probably never that
large so that it would matter much.

Anyway, thanks for answering and sorry for the noise,

Michael.

Re: undo-auto--undoable-change infloop

[Madhu]

* Michael Heerdegen <87y1vwq0su.fsf @web.de> :
Wrote on Wed, 10 Aug 2022 04:39:45 +0200:

> Stefan Monnier <monnier @iro.umontreal.ca> writes:
>
>> But I think you can `cancel-timer` before `timer-activate`, which will
>> make sure it's not in the list any more.
>
> That solution also came to my mind.  Wastes a lot of time, though.  The
> `timer-list' is traversed 3 times: First, to find the timer to delete
> it, second to test if it's already present when trying to add it (your
> change), third, to sort it in at the right position (though I don't
> understand why that is necessary).  The list is probably never that
> large so that it would matter much.

I didn't spot a bug report but I see this commit:

* commit eb7fe81e6db8d630521098a728713e10c9d59c74
|Author:     Stefan Monnier <monnier@iro.umontreal.ca>
|AuthorDate: Fri Aug 5 10:38:59 2022 -0400
|Commit:     Stefan Monnier <monnier@iro.umontreal.ca>
|CommitDate: Fri Aug 5 10:38:59 2022 -0400
|
|    timer.el: Avoid repeated timers
|
|    https://mail.gnu.org/archive/html/emacs-devel/2022-07/msg01127.html
|    points out that end-users can get bitten by this, accidentally
|    calling `timer-activate` on an already activated timer.
|
|    * lisp/emacs-lisp/timer.el (timer--activate): Signal an error if we try
|    to re-add a timer that's already on the timer-list.

FYI

I thought I had a valid use case for calling timer--activate on an
already active timer.

```
(defvar $jps-reader-reset-interval 10)

(defvar $current-search-timer
  (run-with-timer $jps-reader-reset-interval nil
		  (lambda ()
		    (let ((orig $current-search-string))
		      (setq $current-search-string "")
		      (let (message-log-max)
			(message "timer: reset %s to %s" orig
				 $current-search-string))))))

(defun respool ()
  (progn
    ;;(find $current-search-timer timer-list)
    (setf (timer--time $current-search-timer)
	  (time-add (seconds-to-time jps-reader-reset-interval) (current-time)))
     (timer-activate $current-search-timer)))
```

The idea was to handle a "mode" where keystrokes would enter characters
in a "search string". the "mode" would timeout after a 10 seconds. If a
new character of the "search string" is read within the timeout period,
the timer is renewed ("respooled"). If a non "search string" character
is read, then the timer is cancelled and the "mode" exits.

After this commit the last line of respool has to look like

(timer-activate $current-search-timer nil (cancel-timer-internal
  $current-search-timer))


Which uses an internal function, but I thought my timer-activate was
legitimate to extend the timer.  Maybe I should have approached the
problem differently?

timer.el API (was: undo-auto--undoable-change infloop)

[Stefan Monnier]

> I thought I had a valid use case for calling timer--activate on an
> already active timer.
[...]
> Which uses an internal function, but I thought my timer-activate was
> legitimate to extend the timer.  Maybe I should have approached the
> problem differently?

Sigh!  `timer.el` has no doc at all (other than the docstrings) and
almost all the definitions are named "normally" (only
`cancel-timer-internal` and `internal-timer-start-idle` are somehow
labeled as internal), so it's a mess.

My reading of the `timer.el` code and the way it's used
"out there" leads me to think that `timer-activate` should be
considered internal.

And its effect is very poorly defined in its docstring.  So whichever
behavior we choose for it, I'm sure someone can come up with
a legitimate case where that precise behavior is indeed needed.

Someone™ should probably take a step back, decide what the exposed API
*should* be, rename the rest to use internal names, and then add
obsolete aliases to temporarily preserve backward compatibility.


        Stefan

Re: timer.el API

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> And its effect is very poorly defined in its docstring.  So whichever
> behavior we choose for it, I'm sure someone can come up with
> a legitimate case where that precise behavior is indeed needed.

Madhu wrote:

> The idea was to handle a "mode" where keystrokes would enter characters
> in a "search string". the "mode" would timeout after a 10 seconds. If a
> new character of the "search string" is read within the timeout period,
> the timer is renewed ("respooled"). If a non "search string" character
> is read, then the timer is cancelled and the "mode" exits.

This is exactly the scheme I need now and then, and the only kind of
task where I wanted `timer-activate'.

> Someone™ should probably take a step back, decide what the exposed API
> *should* be, rename the rest to use internal names, and then add
> obsolete aliases to temporarily preserve backward compatibility.

Maybe renaming `timer-activate' to `timer--activate' and implementing a
solution for the above kind of requirement would already be a big part
of the way?

Michael.

Re: timer.el API

[Stefan Monnier]

Michael Heerdegen [2022-10-03 22:59:18] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> And its effect is very poorly defined in its docstring.  So whichever
>> behavior we choose for it, I'm sure someone can come up with
>> a legitimate case where that precise behavior is indeed needed.
>
> Madhu wrote:
>> The idea was to handle a "mode" where keystrokes would enter characters
>> in a "search string". the "mode" would timeout after a 10 seconds. If a
>> new character of the "search string" is read within the timeout period,
>> the timer is renewed ("respooled"). If a non "search string" character
>> is read, then the timer is cancelled and the "mode" exits.
> This is exactly the scheme I need now and then, and the only kind of
> task where I wanted `timer-activate'.

You don't need `timer-activate` for that: just deactivate your original
timer with `cancel-timer` and create a new one with
`run-with-idle-timer` if you need that.
Both of which are documented in the ELisp manual, contrary to
`timer-activate`.


        Stefan

Re: timer.el API

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> You don't need `timer-activate` for that: just deactivate your original
> timer with `cancel-timer` and create a new one with
> `run-with-idle-timer` if you need that.
> Both of which are documented in the ELisp manual, contrary to
> `timer-activate`.

I had understood that.  Thus my conclusion that degrading
`timer-activate` to an internal function would be fine for me.

And I also wonder if we would want a new thing like e.g.

(defun postpone-timer (timer seconds)
"Reset the delay of timer to SECONDS.
Activate TIMER if inactive."
    ...)

to simplify the implementation of things like what Madhu described as
his example "mode".

Michael.

Re: undo-auto--undoable-change infloop

[Stefan Monnier]

> After this commit the last line of respool has to look like
>
> (timer-activate $current-search-timer nil (cancel-timer-internal
>   $current-search-timer))
>
> Which uses an internal function,

Just call `cancel-timer` instead of `cancel-timer-internal` and the
problem is solved.


        Stefan

Re: undo-auto--undoable-change infloop

[Madhu]

*  Stefan Monnier <jwvzge74ay0.fsf-monnier+emacs@gnu.org>
Wrote on Fri, 07 Oct 2022 16:50:54 -0400

>> After this commit the last line of respool has to look like
>>
>> (timer-activate $current-search-timer nil (cancel-timer-internal
>>   $current-search-timer))
>>
>> Which uses an internal function,
>
> Just call `cancel-timer` instead of `cancel-timer-internal` and the
> problem is solved.

(CANCEL-TIMER returns NIL)

Earlier it was possible to avoid unnecessary consing, which is now
imposed on the user by the new API.  The new API may still be usable
but is gratuitously inefficient: I think the public/private API purity
fixation which motivated it is misguided here.

Re: undo-auto--undoable-change infloop

[Stefan Monnier]

> (CANCEL-TIMER returns NIL)
>
> Earlier it was possible to avoid unnecessary consing, which is now
> imposed on the user by the new API.  The new API may still be usable
> but is gratuitously inefficient: I think the public/private API purity
> fixation which motivated it is misguided here.

We'll just have to agree to disagree on this.


        Stefan