[Python] Handle py::call_method error without mutating internal state #11840
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BewareMyPower
added
component/python
type/bug
BewareMyPower
requested review from
jerrypeng,
aahmed-se,
jiazhai,
merlimat,
rdhabalia and
sijie
|
It looks like |
jiazhai
approved these changes
Aug 31, 2021
codelipenghui
pushed a commit
that referenced
this pull request
Sep 9, 2021
…#11840) Fixes #11823 When the Python logger is customized with underlying `LoggerWrapper` objects, sometimes `async` Python functions may return an incorrect value like `None`. It's because there's a bug (or feature?) of Boost-python that `py::call_method` will fail in C++ object's destructor. See boostorg/python#374 for details. For the code example in #11823, it's because in `ConsumerImpl`'s destructor, the logger for `AckGroupingTrackerEnabled` will be created again because the logger is thread local and will be created in new threads. In this case, `py::call_method` in `LoggerWrapper#_updateCurrentPythonLogLevel` will fail, and `PyErr_Print` will be called and the error indicator will be cleared, which leads to the result that `async` functions' result became `None`. - Reduce unnecessary `Logger.getEffectiveLevel` calls to get Python log level , just get the log level when the logger factory is initialized and pass the same level to all loggers. - Remove the `PyErr_Print` calls in `LoggerWrapper` related code. In the cases when `py::call_method` failed, use the fallback logger to print logs. - Add a dependent test for custom logger test because once the `LoggerFactory` was set all other tests would be affected. - [x] Make sure that the change passes the CI checks. This change added test `CustomLoggingTest`. Since `asyncio` module was introduced from Python 3.3 while CI is based on Python 2.7, this test cannot be tested by CI unless Python3 based CI was added. (cherry picked from commit 9153e71)
bharanic-dev
pushed a commit
to bharanic-dev/pulsar
that referenced
this pull request
Mar 18, 2022
…apache#11840) Fixes apache#11823 ### Motivation When the Python logger is customized with underlying `LoggerWrapper` objects, sometimes `async` Python functions may return an incorrect value like `None`. It's because there's a bug (or feature?) of Boost-python that `py::call_method` will fail in C++ object's destructor. See boostorg/python#374 for details. For the code example in apache#11823, it's because in `ConsumerImpl`'s destructor, the logger for `AckGroupingTrackerEnabled` will be created again because the logger is thread local and will be created in new threads. In this case, `py::call_method` in `LoggerWrapper#_updateCurrentPythonLogLevel` will fail, and `PyErr_Print` will be called and the error indicator will be cleared, which leads to the result that `async` functions' result became `None`. ### Modifications - Reduce unnecessary `Logger.getEffectiveLevel` calls to get Python log level , just get the log level when the logger factory is initialized and pass the same level to all loggers. - Remove the `PyErr_Print` calls in `LoggerWrapper` related code. In the cases when `py::call_method` failed, use the fallback logger to print logs. - Add a dependent test for custom logger test because once the `LoggerFactory` was set all other tests would be affected. ### Verifying this change - [x] Make sure that the change passes the CI checks. This change added test `CustomLoggingTest`. Since `asyncio` module was introduced from Python 3.3 while CI is based on Python 2.7, this test cannot be tested by CI unless Python3 based CI was added.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #11823
Motivation
When the Python logger is customized with underlying
LoggerWrapperobjects, sometimesasyncPython functions may return an incorrect value likeNone. It's because there's a bug (or feature?) of Boost-python thatpy::call_methodwill fail in C++ object's destructor. See boostorg/python#374 for details.For the code example in #11823, it's because in
ConsumerImpl's destructor, the logger forAckGroupingTrackerEnabledwill be created again because the logger is thread local and will be created in new threads. In this case,py::call_methodinLoggerWrapper#_updateCurrentPythonLogLevelwill fail, andPyErr_Printwill be called and the error indicator will be cleared, which leads to the result thatasyncfunctions' result becameNone.Modifications
Logger.getEffectiveLevelcalls to get Python log level , just get the log level when the logger factory is initialized and pass the same level to all loggers.PyErr_Printcalls inLoggerWrapperrelated code. In the cases whenpy::call_methodfailed, use the fallback logger to print logs.LoggerFactorywas set all other tests would be affected.Verifying this change
This change added test
CustomLoggingTest. Sinceasynciomodule was introduced from Python 3.3 while CI is based on Python 2.7, this test cannot be tested by CI unless Python3 based CI was added.