-
Notifications
You must be signed in to change notification settings - Fork 26.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(3.2): triple AbstractServerCallListener NPE #14009
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 3.2 #14009 +/- ##
==========================================
- Coverage 70.43% 70.41% -0.03%
==========================================
Files 1607 1607
Lines 70074 70097 +23
Branches 10100 10103 +3
==========================================
Hits 49356 49356
- Misses 16080 16095 +15
- Partials 4638 4646 +8 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@icodening @EarthChen PTAL |
if (r.hasException()) { | ||
doOnResponseHasException(r.getException()); | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this case, why response.hasException
is false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
因为直接invoke的response是同步执行的结果,whenCompleteWithContext里面回调的response是异步的AppResponse,只要provider的实现用了异步,在实际的异步结果里面有异常,就会是外面Invoker的response没异常,whenCompleteWithContext回调的appResponse有异常
Because the response of direct invoke is the result of synchronous execution, the response of the callback in whenCompleteWithContext is an asynchronous AppResponse. As long as the provider implementation uses asynchronous, if there is an exception in the actual asynchronous result, it will be because the response of the external Invoker is not abnormal, and the callback of whenCompleteWithContext is appResponse has exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
嗯嗯,那这样看来使用r.hasException()
替换掉原来的response.hasException()
应该会更好,因为r才是最终真实的结果
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK,我改一下
|
* fix NPE * Optimize the handling of exceptions in response --------- Co-authored-by: caoyanan <caoyanan@growingio.com>
@@ -62,14 +62,18 @@ public void invoke() { | |||
final long stInMillis = System.currentTimeMillis(); | |||
try { | |||
final Result response = invoker.invoke(invocation); | |||
if (response.hasException()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
我想问下这边考虑调用一下 hasException 的原因是啥
What is the purpose of the change
NPE bug fix
Brief changelog
response.getValue is null, onReturn(null) will throw Serialize triple response failed NPE
Verifying this change
Checklist