Refactor worker thread capture procedures as internal functions. #1971
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1971 +/- ##
=======================================
Coverage 83.45% 83.45%
=======================================
Files 311 311
Lines 54556 54560 +4
Branches 11491 11529 +38
=======================================
+ Hits 45529 45535 +6
+ Misses 7798 7797 -1
+ Partials 1229 1228 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Pcap++/src/PcapLiveDevice.cpp
Outdated
| catch (const std::exception& ex) | ||
| { | ||
| PCPP_LOG_ERROR("Exception occurred while invoking packet arrival callback: " << ex.what()); | ||
| } | ||
| catch (...) | ||
| { | ||
| PCPP_LOG_ERROR("Unknown exception occurred while invoking packet arrival callback"); | ||
| } |
There was a problem hiding this comment.
I wonder if we should return when an exception is thrown?
Exceptions slow down high performance applications like this...
There was a problem hiding this comment.
The catch block logs an error and essentially terminates the hander's current context as there isn't any other code after it. Terminating the entire entire receive loop would be wrong as the error is only local to the currently received packet.
As far as I know, in most implementations try-catch blocks have little to no performance impact if no exception happens.
There was a problem hiding this comment.
What I mean is that we shouldn't catch the exception in onPacketArrivesCallback but rather in captureThreadMain after calling pcap_dispatch, and terminate the capture thread. The reason is that catching exceptions slows down the packet capture. We can let the users catch and handle exceptions in context->callback if they choose to
There was a problem hiding this comment.
What I mean is that we shouldn't catch the exception in
onPacketArrivesCallbackbut rather incaptureThreadMainafter callingpcap_dispatch, and terminate the capture thread.
The referenced issue overrides that. We are crossing a C to C++ function boundary as the callback pointer is invoked from C code.
We have to catch the exception before returning from the handler. The C compiled code might or might not know what to do with a thrown exception, but ultimately, we have no guarantee, so it's unsafe.
What we do with the exception after catching is irrelevant.
The reason is that catching exceptions slows down the packet capture. We can let the users catch and handle exceptions in
context->callbackif they choose to
I would argue that stopping the capture thread on the first encountered exception in the loop slows down the packet capture even more (to 0).
The nominal fast path case should be that the no exceptions happen / reach outside of the user callback to pcpp lib code.
IMO, if the user callback is throwing exceptions everywhere for some reason, slow capture is still better than no capture.
There was a problem hiding this comment.
one that doesn't handle exceptions (which is the current behavior)
By the current behaviour, did you mean the base behaviour or the current PR state?
Coz by not having any issues, I meant the current PR state.
There was a problem hiding this comment.
I meant the base behavior (before this PR). We don't handle exceptions there...
There was a problem hiding this comment.
That does leave the issue of UB as the exception is going through the C code, tho. :/
There was a problem hiding this comment.
Yes, but we can handle that in the second PR, no?
There was a problem hiding this comment.
Updated 6d03945
Added a TODO reminder tho.
Pcap++/src/PcapLiveDevice.cpp
Outdated
| catch (const std::exception& ex) | ||
| { | ||
| PCPP_LOG_ERROR("Exception occurred while invoking packet arrival callback: " << ex.what()); | ||
| } | ||
| catch (...) | ||
| { | ||
| PCPP_LOG_ERROR("Unknown exception occurred while invoking packet arrival callback"); | ||
| } |
There was a problem hiding this comment.
Is there any reason to catch an exception in this case?
There was a problem hiding this comment.
Propagating an exception from C++ compiled code to C compiled code is considered unsafe. We have to catch any and all exceptions in the handler before returning.
There was a problem hiding this comment.
ditto - we can catch exceptions in captureThreadMainAccumulator and terminate the thread
|
@Dimi1010 this PR is approved. Should we merge it? |
2 participants
Split of #1838
Moves the machinery used when packets are collected on a separate worker thread to internal linkage functions.