fix: fix BatchSpanProcessor with non-runtime exception#4402
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4402 +/- ##
============================================
- Coverage 89.94% 89.91% -0.04%
+ Complexity 4908 4815 -93
============================================
Files 567 553 -14
Lines 15199 14962 -237
Branches 1465 1444 -21
============================================
- Hits 13671 13453 -218
+ Misses 1058 1043 -15
+ Partials 470 466 -4
Continue to review full report at Codecov.
|
| logger.log(Level.FINE, "Exporter failed"); | ||
| } | ||
| } catch (RuntimeException e) { | ||
| } catch (Throwable e) { |
There was a problem hiding this comment.
There was a problem hiding this comment.
I think we should not propagate the exception, the worker thread will be killed as usual if call ThrowableUtil.propagateIfFatal.
There was a problem hiding this comment.
Yes for fatal exceptions like OutOfMemory this needs to happen to let the runtime know of the unrecoverable problem. We always propagateIfFatal in this sort of pattern.
There was a problem hiding this comment.
OK, it sounds reasonable.
| } | ||
| } catch (RuntimeException e) { | ||
| logger.log(Level.WARNING, "Exporter threw an Exception", e); | ||
| } catch (Throwable t) { |
There was a problem hiding this comment.
Can you add a test case with a non-runtime exception? I think Mockito will allow throwing a non-runtime exception from a mock even if not declared, or otherwise there is this pattern
There was a problem hiding this comment.
I have tried to add a unit test that made exporter throw a non-runtime exception. However mockito alert that checked exception is invalid for this method!. Thanks for your suggestion, I will try again.
There was a problem hiding this comment.
Done. I add a unit test with a non-runtime exception.
| when(mockSpanExporter.export(anyList())) | ||
| .thenAnswer( | ||
| invocation -> { | ||
| throw new Exception("No export for you."); |
There was a problem hiding this comment.
Ah that was even easier than expected, nice
4 participants
Fix
BatchSpanProcessorworker thread is killed by non-runtime exception.close #4348