Open AlessandroBono opened 5 years ago
Well, self._post_send calls the local callback method. So it's basically, "Check to see if procmon reported failures. If not, call our callback method, then check to see if procmon reported any new failures." At least that's what it looks like.
That said, the code is a bit convoluted.
Thank you, now I understand why it is like that. The local callback method can be seen as an "active check" while the procmon is a "passive check". The callback check is active because it may send data to the target and see if the received response is the one expected. The procmon check just observes the target without interacting with it. The active check may cause a crash due to the previously fuzzed node, thus it makes sense to perform a passive check again.
However, if no callback check is set, the procmon check is called twice anyway even if nothing happens between the two calls. Probably the second call can be avoided if no callback is set. What do you think about that?
Good point! It would indeed be more efficient not to call the procmon check a second time if nothing has happened.
So, post_send
can have at least two different purposes:
And the way the code was set up (prior to #385), we called procmon's "Flag a failure" post_send method. Then we call all the other post_send
methods in case there's one that will "cause a failure".
A potential solution would be to separate out these methods. E.g.: post_send
performs arbitrary behavior that may have side effects including causing a failure; check_failures
will report any detected failures, but may not have side effects that would (in the normal case) induce a failure. This change alone may provide enough clarity to be worth it.
Now, if we wanted to get really fine-grained, we would call check_failures
in between each post_send
call, since in theory any of them could cause a failure. There is a potential for performance impact here (especially for remote monitors where each check_failures
call is a TCP round trip), but the plus side is that you get more precise chronological data. Options to mitigate performance impact:
post_send
call is just a dummy call with no implementation. In that case, don't check for failures again. But when the post_send really does something, check for failures. Not sure how to do this off-hand:
IMonitorPostSend
(seems ugly). Then you can type-check each monitor before calling post_send
.NotImplementedException
and always catch it (performance impact since the normal path involves throwing an exception, not sure if said performance impact will matter).
After each test case the Session calls
target.procmon.post_send()
twice. The root cause seems to be here. Basically it says: "calltarget.procmon.post_send()
. If no crash has been detected, callself._post_send()
first and thentarget.procmon.post_send()
again". Is it an expected behavior? If so, what is the reasoning behind it?