Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

re-entrancy issue in new pycall! #533

Closed
stevengj opened this issue Aug 21, 2018 · 2 comments
Closed

re-entrancy issue in new pycall! #533

stevengj opened this issue Aug 21, 2018 · 2 comments
Labels

Comments

@stevengj
Copy link
Member

Because the new pycall! implementation in #492 uses a global cache of Python tuples, we encounter some sticky problems due to lack of re-entrancy. In particular, if the PyObject call in pysetarg! itself calls pycall with the same number of arguments, we get the wrong results.

This can happen with the range conversions:

julia> f = py"lambda x,y,z: (x,y,z)"
PyObject <function <lambda> at 0x145cba510>

julia> pycall(f, PyObject, 3,4,5)
PyObject (3, 4, 5)

julia> pycall(f, PyObject, 3:6,4:10,5:11)  # WRONG RESULT:
PyObject (5, 12, range(5, 12))

cc @JobJob

@stevengj
Copy link
Member Author

stevengj commented Aug 21, 2018

On an unrelated note, it looks like the Python arguments never get garbage-collected. PyTuple_SetItem doesn't decref the previous contents, because it is designed to be called exactly once per element on a newly created tuple. I'll try to fix that at the same time.

Correction: PyTuple_SetItem does decref the previous contents, because it calls Py_XSETREF, which calls Py_XDECREF on the old contents. But there is still a problem because this currently only happens on the next function call with the same number of arguments.

@JobJob
Copy link
Contributor

JobJob commented Aug 22, 2018

Nice subtle bug, glad you found it.

As you might recall I did mention the non-threadsafe situation in #483 (comment), but I hadn't considered that PyObject(arg) might call pycall again, meaning re-entry.

Correction: PyTuple_SetItem does decref the previous contents, because it calls Py_XSETREF, which calls Py_XDECREF on the old contents. But there is still a problem because this currently only happens on the next function call with the same number of arguments.

Ahh! Makes sense now.

Anyway, great work as usual, #534 lgtm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants