(bugfix) Unhandled exception - Abort raised when write to the jaeger-…

[ankit-varma10]

…agent fails

Signed-off-by: ankit.varma10 ankit.varma.ub@gmail.com

[codecov]

Codecov Report

Merging #80 into master will increase coverage by 0.1%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master      #80     +/-   ##
=========================================
+ Coverage   88.52%   88.62%   +0.1%     
=========================================
  Files          96       96             
  Lines        2291     2304     +13     
=========================================
+ Hits         2028     2042     +14     
+ Misses        263      262      -1

Impacted Files	Coverage Δ
src/jaegertracing/UDPTransport.h	100% <100%> (ø)	⬆️
src/jaegertracing/UDPTransport.cpp	95% <100%> (+1.12%)	⬆️
src/jaegertracing/utils/UDPClient.h	82.6% <100%> (ø)	⬆️
src/jaegertracing/reporters/RemoteReporter.cpp	75% <0%> (+1.66%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bcd3d3f...855cddb. Read the comment docs.

[isaachier]

Why only logic_error? I'd move this block to the end and replace logic_error with exception. That way it can be a fallback for any exception other than system_error.

[ankit-varma10]

Hey,

Sorry, was caught up in a bunch of other things.

"Why only logic error?"
The only two exceptions I see being raised from UDPClient::emitBatch were logic_error (packet size too big for a single UDP packet) and std::system_error. So, I think I could add one more handler to it for std:: exception.

catch (logic) {
}
catch (system) {

} catch (std:exception) {

}

Thoughts?

[isaachier]

OK cool. My idea what just skip the catch (logic) {} and do the rest as above. logic_error is derived from exception so a final block catches both.

[ankit-varma10]

Oh yeah. I missed that.

Makes sense. I'll make the changes today.

[isaachier]

@ankit-varma10 hello?

[isaachier]

Awesome. I see there is no coverage for this new code. If you don't want to add a test case, I can do it. Just let me know so I can proceed if necessary. Thanks!

[isaachier]

Just an interesting follow up. @manannayak pointed out to me that the RemoteReporter::sweepQueue method should have caught the exception instead of terminating the application. I believed he was correct until I found that RemoteReporter::flush was declared noexcept and throwing, thus violating all guarantees of further exception handling up the stack. Glad someone caught this. Might be worth adding a catch (...) block in RemoteReporter::flush to avoid issues like this in the future.