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. |
|
@icodening @EarthChen PTAL |
| if (r.hasException()) { | ||
| doOnResponseHasException(r.getException()); | ||
| return; | ||
| } |
There was a problem hiding this comment.
in this case, why response.hasException is false？
There was a problem hiding this comment.
因为直接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.
嗯嗯，那这样看来使用r.hasException()替换掉原来的response.hasException()应该会更好，因为r才是最终真实的结果
|
* 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()) { | |||
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