Silent evaluation failures due to too low recursion-limit(100)

[ankostis]

While evaluating correct expressions, I got None as result, but no error were reported (they have silently failed).

When I tried to investigate the issue, I was receiving the following error on Interpreter().eval() from within my debugger (LiClipse + PyDev):

cannot set the recursion limit to 100 at the recursion depth 87: the limit is too low

Obviously my debugger had pushed the stack close to that limit,
and python-interpreter screamed at asteval/asteval.py#L136:

sys.setrecursionlimit(RECURSION_LIMIT)

So I hand-edited asteval/astutils.py:#L16 and increased my limit to i.e. 500:

 RECURSION_LIMIT = 100

But then, all my expressions worked OK!

On first impression, I would consider recursion-limit=100, too low for general use.

My second thought would be to set the limit at least current_stack_depth + 100 and that value(100) to be user-configurable.

But as general precaution, I believe that:

• normal code should not mess with sys.setrecursionlimit() because it might have serious side-effects.

• If this protection measure is needed, user-code should perform that invocation - the documents should explain how.

• At most, an "opt-in" keyword should do that, with something like that:

  import inspect
  
  class Interpreter:
      def __init__(..., recursion_cap=None):
          ...
          if recursion_cap:
              if recursion_cap is True:
                  recursion_cap = DEFAULT_RECURSION_CAP
              recursion_limit = len(inspect.stack()) + recursion_cap
              sys.setrecursionlimit(recursion_limit)
  
  

[newville]

@ankostis @dwelch91 Hm, that seems weird.

First, the failure shouldn't be silent. But I don't know why this didn't raise a RecursionError. Perhaps that was somehow suppressed? That should be fixed for sure.

Limiting the recursion depth was added earlier this year as part #17, and is part of the effort to prevent seg-faulting Python. I don't have a strong opinion on what that actual limit is. With Pythons default of 1000, 100 seems reasonable for asteval, though I suppose 500 is also reasonable. It's also reasonable to say that we should not change this limit at all.

If we do set the value, I think it is better to set this limit and then re-set it back to the previous value for each Interpreter.parse() and/or Interpreter.eval(), and not set it globally with only sys.setrecursionlimit() as that will be set for the entire process, I believe.

The "current stack depth" from len(inspect.stack()) tells you how deep you are into nested function calls. I'm not sure you want to use that as a basis for how much further you want to go -- I think you want to set a hard limit. Like, if you're only in 1 or 2 levels, 100 doesn't matter, but if you're already in 400 levels, do you really want to set the limit to 100 more? I would ask, how the heck did you get to creating an instance of asteval.Interpreter() 400 calls in deep from the top-level of the process.

It would be fine to make this user-configurable. There have been other suggestions for more initialization-time configuration, including what language features to support -- including recursion limit would be fine too. The challenge is that there are a lot of potential configuration options, so it might be best to use something other than arguments to Interpreter.__init__(), say having an InterpreterConfig class that defaulted to many "normal" values, but could be customized before creating an Interpreter.

Updates to code or documentation welcome!

[wayhome]

I had always encounter the following error:

RuntimeError: maximum recursion depth exceeded while calling a Python object

I agree with that 100 is too low.

[ankostis]

@newville while trying to craft a PR, I noticed that sys.setrecursionlimit() gets to be invoked multiple times; twice in Interpreter.eval() and once inside Interpreter.parse().

Is that on purpose?

[ankostis]

Anyhow, #22 is served.

If you want me also to de-duplicate sys.setrecursionlimit() invocations, I can give it a try
(have eval() and parse() delegate their job to private methods that do it without recurse-limit setting).

[ankostis]

Reporting also a non-fatal bug discovered during #22::

The setting/resetting of the recursion-limit happens on every run() and eval() invocation, so at minimum 4x per expression, plus x2 for each extra run() invocation required.