python4delphi icon indicating copy to clipboard operation
python4delphi copied to clipboard

Named parameters in VarPyth not cleared between function calls

Open mce2 opened this issue 5 years ago • 8 comments

Under some circumstances (sorry, I don't have minimal code to reproduce the error) named parameters in VarPyth not cleared between function calls, and function called without named parameters may recieve parameters from previous call with named parameters. Condition check in line 1091 of VarPyth.pas seems to be excessive; in my case commenting it out solved the problem: {if CallDesc^.NamedArgCount > 0 then }GetNamedParams;

mce2 avatar Apr 08 '20 15:04 mce2

I found that this happens in multithread environment: if I call python function with named parameters in one thread, and this call is not finished when I call another function in another thread without named parameters, the second functions receives named parameters from the previous call. Is VarPyth thread safe, and is my way of overcoming the problem safe?

mce2 avatar Apr 08 '20 16:04 mce2

Python is not thread safe. Google for "Python GIL".
Before you try to run python code in threads, make sure you understand python threading and study Demo 33.

pyscripter avatar Apr 08 '20 19:04 pyscripter

I know about GIL, and all my calls are executed between gilstate := PyGILState_Ensure(); PyGILState_Release(gilstate); The python code works fine if I execute it via TPythonEngine.ExecString(). The problem is in VarPyth: it reuses the same TNamedParamArray for all calls, but the former call may be in progress while the latter is started (in my case the former starts wx.App MainLoop, so it does not exit untill the application is finished).

mce2 avatar Apr 09 '20 10:04 mce2

Assumption that one can enter only one function using VarPyth interface, is incorrect. GIL lock may be released and re-acquired inside python code (during time.sleep, IO operations, in GUI message loop, etc.), so in multy-threaded application it's generally normal to have more than one function running inside the same python interpreter.

mce2 avatar Apr 09 '20 12:04 mce2

Can you submit a patch?

pyscripter avatar Apr 09 '20 13:04 pyscripter

I'm not sure if the patch I currently use can cause other problems. I'll try to investigate the problem more thorougly and make a general silution.

mce2 avatar Apr 09 '20 17:04 mce2

I don't see how your PR resolves the issue.

The problem is that fNamedParams is stored as a field of TPythonVariantType (singleton class). If multiple threads use Python Variants to make calls to Python you will still have problems,

Possible Solutions:

  • Make fNamedParams a Global threadvar, but threadvars come with their own performance penalty and limitations.
  • Store fNamedParams in a threadlist with the threadid and retrieve from there.
  • Store fNamedParams in TPythonData (one per variant) instead of TPythonVariantType (singleton class)
  • Store named arguments in the Arguments TVarDataArray, using some marker to separate standard from named arguments.
  • Protect DispInvoke with a Critical Section, so that it cannot be called by a different thread before finishing execution. This seems to be the easiest to implement option.

Why this flawed design

DispInvoke calls DoProcedure and DoFunction which then use EvalPython to do the work. The inherited DispInvoke does not make use of or deal with Named arguments and it needs to be overridden as well as DoProcedure and DoFunction. They all have a fixed signature, so you cannot for example pass another parameter from DispInvoke to DoFunction and you need to have some other means of passing fNamedParams from DispInvoke to DoFunction and then to EvalPython.

pyscripter avatar Apr 09 '20 23:04 pyscripter

You are absolutely right, my code is not a resolution but a dirty workaround. However it looks like GIL will do the magic, and this code should work correct. As I understand VarPyth code, named parameters are transfered via fNamedParams to python dict object by calling PyDict_SetItemString in a loop (line 1676). If I am correct, we may SetLength(fNamedParams, 0) immidiately after this loop. I assume that due to GIL execution of the thread cannot be interrupted between setting fNamedParams and calling PyDict_SetItemString, so this should be OK. This workaround is stinky, but it should work. Sorry, I'm not experienced enough to develop a real solution for the issue.

mce2 avatar Apr 10 '20 09:04 mce2

@mce2 It took quite a while... The solution is similar to what you originally suggested. It also makes sure that fNamedParams is cleared asap and before the python method is executed.

Could you please test it?

pyscripter avatar Nov 02 '23 12:11 pyscripter