[Rd] Handlers in setGraphicsEventHandlers() can recursively call getGraphicsEvent(). Intended behavior?

[Paul Murrell]

Hi

Is the correct patch to remove the setting of the gettingEvent flag or 
would it be better to flip the TRUE/FALSE setting (set to TRUE before 
handling then reset to FALSE after handling) ?

Also, for this patch and for the other two you sent, one difficulty will 
be with testing the patches.  I have no testing code for this, so would 
need at least a test or two from you (ideally someone would also have 
some regression tests, beyond ?getGraphicsEvent, to ensure continuity of 
previous behaviour).

Paul

On 18/09/2016 3:29 a.m., Richard Bodewits wrote:
> Hey all.
>
> As in general it's a bad idea to allow an event handler to generate an
> event, and as comments in the code seem to suggest this isn't the
> intention either, I was wondering about recursion in getGraphicsEvent().
> In main/gevents.c, both doMouseEvent() and doKeybd() have the following
> line;
>
>     dd->gettingEvent = FALSE; /* avoid recursive calls */
>
> And they reset it to TRUE before returning. The effective result of this
> is that the event handlers on the R side are allowed to call
> getGraphicsEvent(), and recurse until they eventually would run out of
> stack space. Though R does catch and handle that cleanly with the error;
>
>     Error: evaluation nested too deeply: infinite recursion /
> options(expressions=)?
>
> A quick scan of the SVN logs suggests this code has been untouched since
> its introduction in 2004, so I'm left to wonder if this is intended
> behavior. It stands out as a bit of a sore thumb due to the generic
> check for recursion in do_getGraphicsEvent() in the same file, which
> will error out with error(_("recursive use of 'getGraphicsEvent' not
> supported")) if dd->gettingEvent is already set to TRUE. Which would
> suggest recursively calling it is very much not intended to be possible.
>
> To me, setting gettingEvent to FALSE seems like an easy mistake to make
> if you temporarily interpret gettingEvent to mean that event(s) are
> allowed to still come in. But the actual interpretation in
> do_getGraphicsEvents() is the opposite, as it's interpreted as an
> indicator of whether or not an event is currently being processed.
>
> I've removed the gettingEvent altering lines from both doMouseEvent()
> and doKeybd() to no ill effect, and doing so disabled the ability to
> call getGraphicsEvent() from inside one of the assigned handlers as
> expected. But after 12 (!) years, it's conceivable that people have come
> to depend on this behavior in existing scripts. Is this something that
> should be left alone to minimize disruption? Or should this be fixed (if
> it is indeed unintended) for the sake of protecting people from infinite
> recursions?
>
> I've included a small patch as attachment that removes the offending lines.
>
>
> - Richard Bodewits
>
>
>
> ______________________________________________
> R-devel at r-project.org mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel
>

-- 
Dr Paul Murrell
Department of Statistics
The University of Auckland
Private Bag 92019
Auckland
New Zealand
64 9 3737599 x85392
paul at stat.auckland.ac.nz
http://www.stat.auckland.ac.nz/~paul/