hydroflask added the comment:
This is also a problem in pysqlite_connection_close() as currently implemented.
--
___
Python tracker
<https://bugs.python.org/issue42
hydroflask added the comment:
Nevermind it seems that it's legal to call Py_BEGIN_ALLOW_THREADS in
tp_dealloc. The fix is then to allow threads around sqlite3_close().
--
___
Python tracker
<https://bugs.python.org/issue42
New submission from hydroflask :
pysqlite_connection_dealloc() calls sqlite3_close{,_v2}(). This can cause a
deadlock if sqlite3_close() tries to acquire a lock that another thread holds,
due to a deadlock between the GIL and an internal sqlite3 lock.
This is especially common with sqlite3
Change by hydroflask :
--
components: +Extension Modules
___
Python tracker
<https://bugs.python.org/issue42698>
___
___
Python-bugs-list mailing list
Unsub
hydroflask added the comment:
Another comment: if calling sqlite3_close() outside of GIL, then the associated
SQL function destructors must take the GIL before calling Py_DECREF
--
___
Python tracker
<https://bugs.python.org/issue42
hydroflask added the comment:
You did the right thing in terms of editing the code.
I was not able to get a deadlock with this code which I think should be
representative of the error. In production the error was happening after 30
minutes of so.
The major problem is that I don't exactly
hydroflask added the comment:
>> The major problem is that I don't exactly know how to provoke SQLite to
>> acquire an internal lock.
> IIRC, you can provoke the internal SQLite lock simply by using transaction
> control: BEGIN (lock) => COMMIT / ROLLBACK (unlock).
Ah
hydroflask added the comment:
Unfortunately I am currently unable to write a repro for this bug but I am 100%
certain it exists from analyzing the code. In our application (which is a large
multithreaded application) after I made the changes consistent with this report
the deadlock vanished
New submission from hydroflask :
_CallPythonObject() in Modules/_ctypes/callbacks.c still uses the old API
(PyObject_CallObject()) to invoke methods. It should use
_PyObject_Vectorcall(). Creating an issue for this since this method is a
little more performance sensitive than normal Python
New submission from hydroflask :
_destructor in connection.c in Python 3.10+ now calls `PyGILState_Ensure()`,
this is a problem because if the destructor is being called while the thread is
being torn down it will cause an unbalanced/erroneous call to
"PyEval_RestoreT
hydroflask added the comment:
If you connect to the sqlite3 database in the example without using the
factory, it will simply segfault.
--
___
Python tracker
<https://bugs.python.org/issue47
hydroflask added the comment:
If you connect with check_same_thread=False, I believe the issue may still
present itself on 3.11+
--
___
Python tracker
<https://bugs.python.org/issue47
hydroflask added the comment:
If PyGILState_Ensure() has been fixed to become re-entrant during
PyGILState_Release() in 3.11+ then I believe that change should be backported
to 3.10. @vstinner would know
--
___
Python tracker
<ht
hydroflask added the comment:
I don't see it immediately but I think it's still possible to happen since all
the same offending code is in place. There are two reasosn why it probably
doesn't happen in 3.11+:
1) because something is deferring calling the finalizer for the zero-ref object
hydroflask added the comment:
> All callbacks triggered from external libraries must protect Python C API
> calls. You may call it offending, but that is the reality; a callback may
> trigger at any point in time, so we need to make sure the GIL is held before
> callin
hydroflask added the comment:
So what is causing this bug in 3.10 is the following stack:
thread_entry_point()
PyGILState_Ensure()
PyGILState_Release()
...
PyGILState_Ensure()
The core issue is that PyGILState_Ensure is getting called while the tstate
itself is in the process
hydroflask added the comment:
Closing this issue since sqlite3_close{,_v2}() is no longer called with the GIL
held in 3.11+
--
stage: -> resolved
status: open -> closed
___
Python tracker
<https://bugs.python.org/i
hydroflask added the comment:
Just to clarify further, the original benchmark by corona10 did indeed lead to
`_CallPythonObject` being invoked but it was not quite the normal use-case. In
the original benchmark it invoked the generated ctypes thunk via Python so as
vstinner said
hydroflask added the comment:
> A benchmark on calling C functions using ctypes sounds better than a
> benchmark calling Python functions.
Calling C functions from Python is not the code path handled by
_CallPythonObject() so no difference in run-time would theoretically be
ob
hydroflask added the comment:
I don't have github so I can't comment there but just as a nitpick, alloca()
never returns NULL, so you don't have to check that. I know that is
inconsistent with its use in callproc.c so for consistency's sake the NULL
check should stay but I would remove both
hydroflask added the comment:
Two things, one is a nit pick the other is more serious. I think vstinner
mentioned this in his review of your patch but on this line:
https://github.com/python/cpython/commit/b5527688aae11d0b5af58176267a9943576e71e5#diff
hydroflask added the comment:
@corona10
I really hope I am not being annoying at this point :) One final nit, in this
line:
https://github.com/python/cpython/pull/31224/files#diff-706e65ee28911740bf638707e19578b8182e57c6a8a9a4a91105d825f95a139dR168
You do not have to check if nargs >
hydroflask added the comment:
For reference, here are MSDN, Linux, OpenBSD, and GCC docs on alloca:
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/alloca?view=msvc-170
https://www.man7.org/linux/man-pages/man3/alloca.3.html
https://man.openbsd.org/alloca.3
https
New submission from hydroflask :
`_ctypes_simple_instance` in _ctypes.c returns the opposite logic of what its
documentation claims. It is supposed to return true when the argument (a type
object) is a direct subclass of `Simple_Type` (`_SimpleCData` in Python code).
However it returns false
hydroflask added the comment:
I place that patch into the public domain, I claim no ownership over that
patch. The patch is attached purely for demonstration purposes.
--
___
Python tracker
<https://bugs.python.org/issue46
hydroflask added the comment:
Thanks again everyone, very much appreciated.
--
___
Python tracker
<https://bugs.python.org/issue46323>
___
___
Python-bugs-list m
hydroflask added the comment:
Was reviewing the code and noticed that a double-free was introduced in the
recent changes:
https://github.com/python/cpython/blob/dd76b3f7d332dd6eced5cbc2ad2adfc397700b3d/Modules/_ctypes/callbacks.c#L192
That line should have been removed in
https
hydroflask added the comment:
Ignore the previous comment :)
--
___
Python tracker
<https://bugs.python.org/issue46323>
___
___
Python-bugs-list mailin
hydroflask added the comment:
Any update on this? I know you wanted to repro but even in the absence of the
repro, I think calling sqlite3_close() without releasing the GIL is
error-prone. If there is no immediate plan to make this change you may close
the issue
hydroflask added the comment:
Easy one to make, might be worth adding a test that would have exercised that
code path.
--
___
Python tracker
<https://bugs.python.org/issue46
hydroflask added the comment:
Another bug, the array returned by alloca() should be zero-initialized. If an
early return happens because of an intermediate error, Py_DECREF() will be
called on uninitialized memory. Also it should be Py_XDECREF() I think
hydroflask added the comment:
@corona10
Thank you for looking into it.
The benchmark you did is not reflective of my use-case. I have a codebase that
I have completely converted into a C extension using mypyc. So the ctypes
callback does not call into interpreted Python code but actually
hydroflask added the comment:
Okay I put together a benchmark that better exemplifies my use case and results
are in favor of adding patch.
When bench_callback_v2.py is not compiled with mypyc I see a ~10% reduction in
runtime:
$ unpatched-env/bin/python bench_callback_v2.py
hydroflask added the comment:
Vanilla bench_callback_v2.py, not compiled with mypyc:
$ env/bin/python bench_callback_v2.py
1.114047016017139
$ env2/bin/python bench_callback_v2.py
0.9644024870358407
~13% reduction in runtime. Nice job
34 matches
Mail list logo
