You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/09/21 02:02:23 UTC
[GitHub] [pulsar] zbentley opened a new pull request, #16535: [fix] [python client] Better Python garbage collection management for C++-owned objects
zbentley opened a new pull request, #16535:
URL: https://github.com/apache/pulsar/pull/16535
Fixes https://github.com/apache/pulsar/issues/16527
### Motivation
Copied from issue discussion:
The problem is the [GIL](https://wiki.python.org/moin/GlobalInterpreterLock). When Python object reference counts are manipulated from compiled code, those manipulations are not atomic or protected by the GIL in any way. Incrementing a refcount is often coincidentally safe to do without the GIL, since the data structures in the Python interpreter that are altered by a refcount-bump are few and not terribly shared. However, decrementing a refcount without the GIL is extremely dangerous; the act of decrementing a refcount can trigger object destruction, which can then trigger more object destruction, and so on: decrementing a refcount triggers an arbitrary number of user functions (destructors) to run in Python, and can trigger wide-ranging changes (including system calls, memory allocation/deallocation--basically anything) across the interpreter's internal state.
Running such operations in true multi-threaded parallel in Python is basically guaranteed to break things. In most cases (I'm guessing here, as I don't know Boost/C++ well), I think the attempt to clean up the reference either blocks or fails in such a way that the C++ runtime won't properly clean up an object, preventing thread reaping from running internally. In some cases, the racy python GC operations overlap with shared interpreter data structures and cause segfaults. In rare cases, "impossible" (i.e. random objects changing type) errors are raised in Python itself, though you may have to run the above snippet for a long time to see one of those.
Such GIL-unprotected refcount manipulation occurs in the Pulsar client [here](https://github.com/apache/pulsar/blob/master/pulsar-client-cpp/python/src/config.cc#L106), [here](https://github.com/apache/pulsar/blob/master/pulsar-client-cpp/python/src/config.cc#L114), [here](https://github.com/apache/pulsar/blob/master/pulsar-client-cpp/python/src/config.cc#L172), and [here](https://github.com/apache/pulsar/blob/master/pulsar-client-cpp/python/src/config.cc#L176), though the first and third may be safe from this condition by dint of the fact that they're only invoked directly from calling Python code which already has the GIL.
### Modifications
- **Done** protected logger handle refcount mutations with the GIL, resolving a segfault risk.
- **Done** resolve thread leak.
- **Done** add tests.
- **Done** don't acquire GIL for logging messages that won't be emitted due to log level.
### Verifying this change
This change added tests and can be verified via the Python client test suite: `python pulsar_test.py`.
- [x] `doc-not-needed`
Bugfix; external contracts do not change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #16535: [fix] [python client] Better Python garbage collection management for C++-owned objects
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16535:
URL: https://github.com/apache/pulsar/pull/16535#discussion_r947521699
##########
pulsar-client-cpp/lib/checksum/crc32c_arm.h:
##########
@@ -37,11 +37,11 @@
#define crc32c_u16(crc, v) __crc32ch(crc, v)
#define crc32c_u32(crc, v) __crc32cw(crc, v)
#define crc32c_u64(crc, v) __crc32cd(crc, v)
-#define PREF4X64L1(buffer, PREF_OFFSET, ITR) \
Review Comment:
Yeah. The `clang-format` version required is now 6 or later. You can simply run `docker-format.sh` to reformat the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zbentley commented on pull request #16535: [fix] [python client] Better Python garbage collection management for C++-owned objects
Posted by GitBox <gi...@apache.org>.
zbentley commented on PR #16535:
URL: https://github.com/apache/pulsar/pull/16535#issuecomment-1226571658
@BewareMyPower fixing that test is proving troublesome. Something happening in the destructor is corrupting the python interpreter state and causing that test to fail. The failure's not a testing issue though; it's a real problem.
I'll keep working on it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zbentley commented on pull request #16535: [fix] [python client] Better Python garbage collection management for C++-owned objects
Posted by GitBox <gi...@apache.org>.
zbentley commented on PR #16535:
URL: https://github.com/apache/pulsar/pull/16535#issuecomment-1251188370
@tuteng done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zbentley commented on a diff in pull request #16535: [fix] [python client] Better Python garbage collection management for C++-owned objects
Posted by GitBox <gi...@apache.org>.
zbentley commented on code in PR #16535:
URL: https://github.com/apache/pulsar/pull/16535#discussion_r931756707
##########
pulsar-client-cpp/lib/checksum/crc32c_arm.h:
##########
@@ -37,11 +37,11 @@
#define crc32c_u16(crc, v) __crc32ch(crc, v)
#define crc32c_u32(crc, v) __crc32cw(crc, v)
#define crc32c_u64(crc, v) __crc32cd(crc, v)
-#define PREF4X64L1(buffer, PREF_OFFSET, ITR) \
Review Comment:
`make format` wanted this diff; perhaps because I installed a newer/older version of clang-format? Regardless, it looks harmless.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #16535: [fix] [python client] Better Python garbage collection management for C++-owned objects
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16535:
URL: https://github.com/apache/pulsar/pull/16535#discussion_r963470828
##########
pulsar-client-cpp/python/pulsar_test.py:
##########
@@ -1248,6 +1250,35 @@ def test_json_schema_encode(self):
second_encode = schema.encode(record)
self.assertEqual(first_encode, second_encode)
+ def test_logger_thread_leaks(self):
+ def _do_connect(close):
+ logger = logging.getLogger(str(threading.currentThread().ident))
+ logger.setLevel(logging.INFO)
+ client = pulsar.Client(
+ service_url="pulsar://localhost:6650",
+ io_threads=4,
+ message_listener_threads=4,
+ operation_timeout_seconds=1,
+ log_conf_file_path=None,
+ authentication=None,
+ logger=logger,
+ )
+ client.get_topic_partitions("persistent://public/default/partitioned_topic_name_test")
+ if close:
+ client.close()
+
+ for should_close in (True, False):
+ assert threading.active_count() == 1, "Explicit close: {}; baseline is 1 thread".format(should_close)
+ _do_connect(should_close)
+ assert threading.active_count() == 1, "Explicit close: {}; synchronous connect doesn't leak threads".format(should_close)
+ threads = []
+ for _ in range(10):
+ threads.append(threading.Thread(target=_do_connect))
Review Comment:
```suggestion
threads.append(threading.Thread(target=_do_connect, args=(should_close)))
```
Should we add the args here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #16535: [fix] [python client] Better Python garbage collection management for C++-owned objects
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16535:
URL: https://github.com/apache/pulsar/pull/16535#discussion_r921956049
##########
pulsar-client-cpp/python/pulsar_test.py:
##########
@@ -17,8 +17,8 @@
# specific language governing permissions and limitations
# under the License.
#
-
-
Review Comment:
Please restore the blank lines (at least one) because the blank line is a part of the license.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on pull request #16535: [fix] [python client] Better Python garbage collection management for C++-owned objects
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on PR #16535:
URL: https://github.com/apache/pulsar/pull/16535#issuecomment-1314658062
could you please cherry-pick this PR to branch-2.9? thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zbentley commented on pull request #16535: [fix] [c++ and python client] Better Python garbage collection management for C++-owned objects
Posted by GitBox <gi...@apache.org>.
zbentley commented on PR #16535:
URL: https://github.com/apache/pulsar/pull/16535#issuecomment-1182452446
WIP update:
- Fixed thread leak by setting `logThreads` to `False` inside the C++ code, which should prevent thread-switching by the Python interpreter. I might be wrong about that, but the worst case behavior is a very occasional thread leak of the sort that occurs 100% of the time now, so this is still an improvement.
- Added unit test.
- Reduced unnecessary GIL-acquire operations that happened when log messages were sent at levels that weren't enabled for logging.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zbentley commented on pull request #16535: [fix] [python client] Better Python garbage collection management for C++-owned objects
Posted by GitBox <gi...@apache.org>.
zbentley commented on PR #16535:
URL: https://github.com/apache/pulsar/pull/16535#issuecomment-1183345941
Assistance requested from a maintainer: the license check is failing, but I didn't add any new files. How can I tell what files it would like me to add a license for?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zbentley commented on pull request #16535: [fix] [c++ and python client] Better Python garbage collection management for C++-owned objects
Posted by GitBox <gi...@apache.org>.
zbentley commented on PR #16535:
URL: https://github.com/apache/pulsar/pull/16535#issuecomment-1181255243
**WIP notes**: the segfault is fixed as expected, but threads now leak if the log level's low enough to result in at least one call to the Python logger's `_log` internal function.
Pull on the thread of "why" and you arrive at an unconditional call to `threading.currentThread` down in Python's logging machinery. If that method is called inside a Pulsar thread, the thread leaks thereafter; somehow, a reference to it is manipulated such that the thread doesn't go away on the system.
Fortunately, there's a way to turn that off. Unfortunately, that's the `logging.logThreads = 1` or `= 0` module global. That's a weird and sad Python config API--not only is there no way to modify it in a race-proof way, but boost-python doesn't make it easy to manipulate the value within the client's C++ code, so the usual `import("logging").attr("logThreads") = object(0)` doesn't seem to work (at least for me, my C++ is basically nonexistent, and my Boost is worse).
The easiest thing I know how to do is write a Python wrapper for the logger that temporarily manipulates that value, which I'l try next.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #16535: [fix] [python client] Better Python garbage collection management for C++-owned objects
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16535:
URL: https://github.com/apache/pulsar/pull/16535#discussion_r963478544
##########
pulsar-client-cpp/python/pulsar_test.py:
##########
@@ -1248,6 +1250,35 @@ def test_json_schema_encode(self):
second_encode = schema.encode(record)
self.assertEqual(first_encode, second_encode)
+ def test_logger_thread_leaks(self):
+ def _do_connect(close):
+ logger = logging.getLogger(str(threading.currentThread().ident))
+ logger.setLevel(logging.INFO)
+ client = pulsar.Client(
+ service_url="pulsar://localhost:6650",
+ io_threads=4,
+ message_listener_threads=4,
+ operation_timeout_seconds=1,
+ log_conf_file_path=None,
+ authentication=None,
+ logger=logger,
+ )
+ client.get_topic_partitions("persistent://public/default/partitioned_topic_name_test")
+ if close:
+ client.close()
+
+ for should_close in (True, False):
+ assert threading.active_count() == 1, "Explicit close: {}; baseline is 1 thread".format(should_close)
+ _do_connect(should_close)
+ assert threading.active_count() == 1, "Explicit close: {}; synchronous connect doesn't leak threads".format(should_close)
Review Comment:
```suggestion
self.assertEqual(threading.active_count(), 1, "Explicit close: {}; baseline is 1 thread".format(should_close))
_do_connect(should_close)
self.assertEqual(threading.active_count(), 1, "Explicit close: {}; synchronous connect doesn't leak threads".format(should_close))
```
Use `assertEquals` so we can know the current thread count if it failed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zbentley commented on pull request #16535: [fix] [python client] Better Python garbage collection management for C++-owned objects
Posted by GitBox <gi...@apache.org>.
zbentley commented on PR #16535:
URL: https://github.com/apache/pulsar/pull/16535#issuecomment-1214239871
Any update on this? To my knowledge it should be mergeable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower merged pull request #16535: [fix] [python client] Better Python garbage collection management for C++-owned objects
Posted by GitBox <gi...@apache.org>.
BewareMyPower merged PR #16535:
URL: https://github.com/apache/pulsar/pull/16535
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on pull request #16535: [fix] [python client] Better Python garbage collection management for C++-owned objects
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #16535:
URL: https://github.com/apache/pulsar/pull/16535#issuecomment-1253112336
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zbentley commented on pull request #16535: [fix] [python client] Better Python garbage collection management for C++-owned objects
Posted by GitBox <gi...@apache.org>.
zbentley commented on PR #16535:
URL: https://github.com/apache/pulsar/pull/16535#issuecomment-1235955753
@BewareMyPower I redid some of this work and pulled more logging related logic into pure Python, which is now passed as a callback function into the C++ code. That ends up having fewer issues, as many of the `boost::python` utility functions (like `import`) themselves run the various `PyErr` functions internally, which mutates the error state that is being relied upon by `asyncio` for that failing test.
The code should be a little simpler now, though the line count went up (sorry).
Additionally, the `PyError_Restore` pattern added for the new logging code should probably be used for other "C++ calls python and shouldn't mess up pre-existing interpreter error state" hooks in `pulsar-client` (e.g. consumer message listeners and the like).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #16535: [fix] [python client] Better Python garbage collection management for C++-owned objects
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16535:
URL: https://github.com/apache/pulsar/pull/16535#discussion_r945397499
##########
pulsar-client-cpp/lib/checksum/crc32c_arm.h:
##########
@@ -37,11 +37,11 @@
#define crc32c_u16(crc, v) __crc32ch(crc, v)
#define crc32c_u32(crc, v) __crc32cw(crc, v)
#define crc32c_u64(crc, v) __crc32cd(crc, v)
-#define PREF4X64L1(buffer, PREF_OFFSET, ITR) \
Review Comment:
@zbentley Could you revert these changes first?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zbentley commented on a diff in pull request #16535: [fix] [python client] Better Python garbage collection management for C++-owned objects
Posted by GitBox <gi...@apache.org>.
zbentley commented on code in PR #16535:
URL: https://github.com/apache/pulsar/pull/16535#discussion_r947479658
##########
pulsar-client-cpp/lib/checksum/crc32c_arm.h:
##########
@@ -37,11 +37,11 @@
#define crc32c_u16(crc, v) __crc32ch(crc, v)
#define crc32c_u32(crc, v) __crc32cw(crc, v)
#define crc32c_u64(crc, v) __crc32cd(crc, v)
-#define PREF4X64L1(buffer, PREF_OFFSET, ITR) \
Review Comment:
@BewareMyPower sure, but as I recall, if I do then the clang-format build fails.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zbentley commented on a diff in pull request #16535: [fix] [python client] Better Python garbage collection management for C++-owned objects
Posted by GitBox <gi...@apache.org>.
zbentley commented on code in PR #16535:
URL: https://github.com/apache/pulsar/pull/16535#discussion_r949377895
##########
pulsar-client-cpp/python/src/utils.h:
##########
@@ -82,3 +82,25 @@ struct CryptoKeyReaderWrapper {
CryptoKeyReaderWrapper();
CryptoKeyReaderWrapper(const std::string& publicKeyPath, const std::string& privateKeyPath);
};
+
+class CaptivePythonObjectMixin {
+ protected:
+ PyObject* _captive;
+
+ CaptivePythonObjectMixin(PyObject* captive) {
+ _captive = captive;
+ PyGILState_STATE state = PyGILState_Ensure();
+ Py_XINCREF(_captive);
+ PyGILState_Release(state);
+ }
+
+ CaptivePythonObjectMixin(py::object captive) : CaptivePythonObjectMixin(captive.ptr()) {}
+
+ ~CaptivePythonObjectMixin() {
+ if (Py_IsInitialized()) {
+ PyGILState_STATE state = PyGILState_Ensure();
+ Py_XDECREF(_captive);
+ PyGILState_Release(state);
+ }
+ }
+};
Review Comment:
Done!
##########
pulsar-client-cpp/lib/checksum/crc32c_arm.h:
##########
@@ -37,11 +37,11 @@
#define crc32c_u16(crc, v) __crc32ch(crc, v)
#define crc32c_u32(crc, v) __crc32cw(crc, v)
#define crc32c_u64(crc, v) __crc32cd(crc, v)
-#define PREF4X64L1(buffer, PREF_OFFSET, ITR) \
Review Comment:
Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #16535: [fix] [python client] Better Python garbage collection management for C++-owned objects
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16535:
URL: https://github.com/apache/pulsar/pull/16535#discussion_r947522848
##########
pulsar-client-cpp/python/src/utils.h:
##########
@@ -82,3 +82,25 @@ struct CryptoKeyReaderWrapper {
CryptoKeyReaderWrapper();
CryptoKeyReaderWrapper(const std::string& publicKeyPath, const std::string& privateKeyPath);
};
+
+class CaptivePythonObjectMixin {
+ protected:
+ PyObject* _captive;
+
+ CaptivePythonObjectMixin(PyObject* captive) {
+ _captive = captive;
+ PyGILState_STATE state = PyGILState_Ensure();
+ Py_XINCREF(_captive);
+ PyGILState_Release(state);
+ }
+
+ CaptivePythonObjectMixin(py::object captive) : CaptivePythonObjectMixin(captive.ptr()) {}
+
+ ~CaptivePythonObjectMixin() {
+ if (Py_IsInitialized()) {
+ PyGILState_STATE state = PyGILState_Ensure();
+ Py_XDECREF(_captive);
+ PyGILState_Release(state);
+ }
+ }
+};
Review Comment:
Could you add a new line at the end of file for all new files?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #16535: [fix] [python client] Better Python garbage collection management for C++-owned objects
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #16535:
URL: https://github.com/apache/pulsar/pull/16535#issuecomment-1220405364
Could you fix this test?
```
======================================================================
FAIL: test_async_func_with_custom_logger (__main__.CustomLoggingTest)
----------------------------------------------------------------------
Traceback (most recent call last):
File "custom_logger_test.py", line 48, in test_async_func_with_custom_logger
self.assertEqual(value, result)
AssertionError: 'foo' != None
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #16535: [fix] [python client] Better Python garbage collection management for C++-owned objects
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16535:
URL: https://github.com/apache/pulsar/pull/16535#discussion_r963460602
##########
pulsar-client-cpp/python/pulsar_test.py:
##########
@@ -1248,6 +1250,35 @@ def test_json_schema_encode(self):
second_encode = schema.encode(record)
self.assertEqual(first_encode, second_encode)
+ def test_logger_thread_leaks(self):
+ def _do_connect(close):
+ logger = logging.getLogger(str(threading.currentThread().ident))
Review Comment:
```suggestion
logger = logging.getLogger(str(threading.current_thread().ident))
```
`currentThread` is deprecated, see https://docs.python.org/3/library/threading.html#threading.current_thread
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zbentley commented on a diff in pull request #16535: [fix] [python client] Better Python garbage collection management for C++-owned objects
Posted by GitBox <gi...@apache.org>.
zbentley commented on code in PR #16535:
URL: https://github.com/apache/pulsar/pull/16535#discussion_r922336450
##########
pulsar-client-cpp/python/pulsar_test.py:
##########
@@ -17,8 +17,8 @@
# specific language governing permissions and limitations
# under the License.
#
-
-
Review Comment:
👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #16535: [fix] [python client] Better Python garbage collection management for C++-owned objects
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #16535:
URL: https://github.com/apache/pulsar/pull/16535#issuecomment-1247814410
@zbentley Could you merge latest master to have some CI fixes?
@merlimat @Demogorgon314 @RobertIndie Could anyone take a second look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower closed pull request #16535: [fix] [python client] Better Python garbage collection management for C++-owned objects
Posted by GitBox <gi...@apache.org>.
BewareMyPower closed pull request #16535: [fix] [python client] Better Python garbage collection management for C++-owned objects
URL: https://github.com/apache/pulsar/pull/16535
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #16535: [fix] [python client] Better Python garbage collection management for C++-owned objects
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16535:
URL: https://github.com/apache/pulsar/pull/16535#discussion_r963442683
##########
pulsar-client-cpp/python/src/utils.h:
##########
@@ -82,3 +82,25 @@ struct CryptoKeyReaderWrapper {
CryptoKeyReaderWrapper();
CryptoKeyReaderWrapper(const std::string& publicKeyPath, const std::string& privateKeyPath);
};
+
+class CaptivePythonObjectMixin {
+ protected:
+ PyObject* _captive;
+
+ CaptivePythonObjectMixin(PyObject* captive) {
+ _captive = captive;
+ PyGILState_STATE state = PyGILState_Ensure();
+ Py_XINCREF(_captive);
+ PyGILState_Release(state);
+ }
+
+ CaptivePythonObjectMixin(py::object captive) : CaptivePythonObjectMixin(captive.ptr()) {}
Review Comment:
```suggestion
```
Remove this override constructor since it's never used currently.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #16535: [fix] [python client] Better Python garbage collection management for C++-owned objects
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #16535:
URL: https://github.com/apache/pulsar/pull/16535#issuecomment-1314681309
@congbobo184 Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org