Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements to <exec> function #423

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dolk13
Copy link
Contributor

@dolk13 dolk13 commented Oct 20, 2019

I added only 'exec verify'.
Improving output tracing brings a lot of changes. I will try to do PR later.

resolve #7

Added 'exec verify' to wait for a return code and verify it
@dolk13 dolk13 force-pushed the feat/improvements_exec_7 branch from d4e2a60 to aa52c00 Compare October 21, 2019 22:11
@wdoekes
Copy link
Member

wdoekes commented Nov 8, 2019

@rkday : something you want to review?

This may need some docs, as documenting in code isn't enough if we want people to find it.

@wdoekes wdoekes added this to the 3.7 milestone Nov 8, 2019
@rkday rkday self-requested a review November 9, 2019 18:29
Copy link
Contributor

@rkday rkday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dolk13 , thanks for this!

As Walter says, the docs need an update to mention verify:

Probably the key thing to mention as a difference between exec command and exec verify is that the latter is synchronous rather than asynchronous - SIPp will wait for the verified command to run before proceeding, and nothing can happen (no SIP messages can be handled, new calls set up, etc.) while it is running.

That makes me somewhat nervous, because I think this implementation is only suitable for very low loads (or very fast commands) - I ran a scenario with a verify-command that took 200ms to run, and despite me requesting a call setup rate of 10/sec it only actually achieved 2.7/sec.

I think a better approach would be:

  • don't call waitpid() in call.cpp
  • instead, pause the call (task.cpp has a setPaused method) and add a pointer to the call to a std::map keyed off the PID
  • at the start of each iteration in traffic_thread in sipp.cpp, call waitpid with the WNOHANG option and a PID of -1, to check of any child process has exited
  • if it has, unpause the call associated with it, and verify the return code

But I think I'd be OK to accept this without this change as long as the impact on the call rate was clearly documented, and improve performance in future.

}
}
break;
}
} else if (currentAction->getActionType() == CAction::E_AT_EXEC_INTCMD) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not still need this INTCMD section?

@rkday rkday mentioned this pull request Nov 10, 2019
@wdoekes wdoekes removed this from the 3.7 milestone Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improvements to <exec> function
3 participants