Add some gc safety around _mysql__fetch_row #348
Conversation
Users of gc.get_referrers() could cause a SystemError to be raised if this function is running in a different python thread.
Codecov Report
@@ Coverage Diff @@
## master #348 +/- ##
=========================================
Coverage ? 86.59%
=========================================
Files ? 12
Lines ? 1537
Branches ? 0
=========================================
Hits ? 1331
Misses ? 206
Partials ? 0
Continue to review full report at Codecov.
|
|
Please detailed information about why and how gc.get_referrers() is broken. |
|
gc.get_referrers is not broken per say, its just not safe in a multi threaded environment when c-api is used. We added this deflect concerns that _mysql.c was broken or python itself had a bug, when the behavior of gc module is well known and not considered a bug by the python core devs. |
|
I said "detailed information"! I don't understand the problem you're saying yet. Only information I got is it's I don't want merge such code without understanding. |
Link to bugs.python.org issue. |
Not that its very useful |
It's very important information. What is your exact Python version? |
|
OK, I understand the error is from It is a dirty part of MySQLdb I dislike. Anyway, thank you for reporting the issue, while I don't like this change. |
MySQLdb/_mysql.c
Outdated
| // This function can get a reference to the tuple r, and if that | ||
| // code is preempted while holding a ref to r, the _PyTuple_Resize | ||
| // will raise a SystemError because the ref count is 2. | ||
| PyObject_GC_UnTrack(*r); |
There was a problem hiding this comment.
Move these code to _mysql_ResultObject_fetch_row.
There was a problem hiding this comment.
its pretty safe everywhere else we do it in the dangerous area where we allow threads.
There was a problem hiding this comment.
It is safe untrack soon after creatin, and track again after _PyTuple_Resize().
There was a problem hiding this comment.
For example, convert_row may execute Python code.
Execute python code means it allows threads switching, Calling gc module, etc.
Py_BEGIN_ALLOW_THREADS is not the only danger API. There are many danger APIs
which can execute any Python code. e.g. Py_DECREF, PyObject_RichCompareBool,
PyList_Append, etc.
Users of gc.get_referrers() could cause a SystemError to be raised if this function is running in a different python thread.
We ended up doing this at Facebook just to be safe, because it was really hard figuring out that it was not a _mysql issue but usage of gc.get_referrers() in a multi-threaded environment. but I understand if you don't feel its needed.