Hold the GIL when incrementing None's refcount to prevent race conditions when there are multiple Python threads

[vasil-pashov]

Reference Issues/PRs

None is a global static object in Python which is also refcounted. When ArcticDB creates None objects it must increase their refcount. It must acquire the GIL when the refcount is increased. Currently we don't acquire the GIL when we do this, we only hold a SpinLock protecting other ArcticDB threads from racing on the GIL refcount. With this change we add an atomic variable in the PythonHandler data which will accumulate the refcount. Then at the end of the operation when we reacquire the GIL we will increase the refcount. The same is done for the NaN refcount, note that we don't really need the GIL to increase NaN's refcount as we create it internally and don't handle it to Python until the read operation is done. Currently only read operations need to work with the None object.

apply_global_refcounts must be called at the very end before passing the dataframe to python to prevent something raising an exception in after the refcount is applied but before python receives the data. Increasing None's refcount but never decreasing it doesn't seem to be fatal but we're trying to be good citizens. The best place for that is adapt_read_df or adapt_read_dfs as they are called at the end of all read functions. The code is changed so that the type handler data is created always in the python bindings file as it's easier to track.

What does this implement or fix?

Any other comments?

Checklist

Checklist for code changes...

• Have you updated the relevant docstrings, documentation and copyright notice?
• Is this contribution tested against all ArcticDB's features?
• Do all exceptions introduced raise appropriate error messages?
• Are API changes highlighted in the PR description?
• Is the PR labelled as enhancement or bug so it appears in autogenerated release notes?

[poodlewars]

Is this and the couple of places in python_bindings.cpp the only places where we create Python None objects in the native layer?

[vasil-pashov]

Yes, read methods are affected: read, read_batch, batch_read_keys, read_index, read_column_stats_version, LibraryTool::read

[poodlewars]

I think we should have tests for all our APIs that can allocate Nones - like batch_read and the things in python_bindings.cpp that you touched as well

[vasil-pashov]

Good idea, batch_read is easy to test. I'm not sure how the other methods like: read_index, batch_read_keys, read_column_stats_version and LibraryTool::read can end up with None values. I suspect that only read and batch_read can contain user defined strings that can be None.

[IvoDD]

Only important comments are with regards to when to call apply_global_refcounts