Some modifications

[fchareyron]

Hi

I join a patch file including some needed modifications to use pysipp properly in my software.

In command.py, some missing SIPp parameters.
In agent.py and init.py, some modifications to handle properly the log dir and to generate error log file.
In report.py, some modifications to avoid seldom exception cases. For any reason, proc.streams.stderr doesn't exist when used.

I hope it's useful enough to be included in the product.
Regards
Frederic

[goodboy]

For one I'd just use a range(5) here instead of the while loop.
@fchareyron Can you explain how this helps?
As far as I remember stderr capture should be synchronous so it shouldn't make any difference how long you wait? Maybe you found something else in your use?

[fchareyron]

In fact, I do not know if the loop is useful. As the occurrence of exceptions is rare, I do not know under what conditions they occur. The loop gives some time to STREAM to be created, but perhaps instead it is already destroyed, or will not be built.
With this change, the exceptions have disappeared.

[goodboy]

@fchareyron ok so I would revert these changes outright since they really aren't solving any problem that I know of.

The stderr getting logged, even if blank, is fine and often helpful when debugging SIPp itself.

[goodboy]

@fchareyron As a starting point you should make sure all the unit tests pass.
As you can see from the Travis job above one of the changes you made breaks one of the tests.

You can run the tests using tox.

[goodboy]

@fchareyron to delete a file with git see git-rm. You can delete the duplicate test_launcher.py that way.

I agree that the error_file being created too slowly is probably a race condition internal to SIPp but I don't think it has anything to do with the way we're killing the process. The whole point behind signalling SIPp to end "gracefully" using SIGUSR1 is that this kind of thing shouldn't happen. I'm not a huge fan of the idea of having a static sleep time inside launch.py as I think it's merely working around a problem that doesn't always exhibit in the same way.

In summary I think the only acceptable changes in this patch set are those made to pysipp/command.py. Everything else is mostly unnecessary or too specific to some odd raciness in SIPp itself.

For your main changes these are my justifications for why they aren't necessary:

• adding error_file collection is unreliable and redundant since stderr collection is already done and should be easy to capture using any sane test framework or by manually accessing proc.streams.stderr.
• adding a logfile kwarg to pysipp.walk() is redundant since it already passes through scenkwargs to pysipp.agent.Scenario here.

If you can revert all the changes except for command.py then I will happily merge. If you're uncomfortable doing that yourself (including a proper git rebase to clean up your history on this branch) I can add the additional flags to command.py for you on the master branch.

Let me know what you prefer.

[fchareyron]

I agree, it is not a good idea to to add a static sleep into the code
just to pass a test.

But I do not agree on the cause of the problem. I do not think SIPp
fails, I rather think that the test is not relevant. The test stops SIPp
before SIPp meets an error condition. It has no reason to generate an
error and to produce the error file expected by the test. With the
modifications that I proposed, I checked that the error logged in the
error file is dated 4 seconds after the first trace of the calldebug file.

I don't agree also with your proposal to only keep command.py modifications.

Without fixes proposed in init.py and agent.py at line 127, the
logdir parameter is not treated, traces are always created in /tmp, not
in the expected directory.

In report.py, you focus on the loop, and I agree, it is probably not
necessary. But the real fix is 3 lines after. The code must check that
'proc' has a 'streams' attribute before using it. If not, pysipp crashes
in rare cases. My proposal is to create a local stderr just to log the
cause.

I don't use git, so I don't know what to rebase. So please, add the
additional flags to command.py on the master branch.

Le 26/07/2016 à 19:50, goodboy a écrit :

@fchareyron https://github.com/fchareyron to delete a file with git
see git-rm https://git-scm.com/docs/git-rm. You can delete the
duplicate |test_launcher.py| that way.

I agree that the |error_file| being created too slowly is probably a
race condition internal to SIPp but I don't think it has anything to
do with the way we're killing the process. The whole point behind
signalling |SIPp| to end "gracefully" using |SIGUSR1| is that this
kind of thing shouldn't happen. I'm not a huge fan of the idea of
having a static sleep time inside |launch.py| as I think it's merely
working around a problem that doesn't always exhibit in the same way.

In summary I think the only acceptable changes in this patch set are
those made to |pysipp/command.py|. Everything else is mostly
unnecessary or too specific to some odd raciness in |SIPp| itself.

For your main changes these are my justifications for why they aren't
necessary:

• adding |error_file| collection is unreliable and redundant since
  |stderr| collection is already done and should be easy to capture
  using any sane test framework or by manually accessing
  |proc.streams.stderr|.
• adding a |logfile| kwarg to |pysipp.walk()| is redundant since it
  already passes through |scenkwargs| to |pysipp.agent.Scenario|
  here
  https://github.com/fchareyron/pysipp/blob/c32bd3e41ddc27f3b597e3ebc5fe1385055cff1f/pysipp/__init__.py#L172.

If you can revert all the changes except for |command.py| then I will
happily merge. If you're uncomfortable doing that yourself (including
a proper git rebase https://git-scm.com/docs/git-rebase to clean up
your history on this branch) I can add the additional flags to
|command.py| for you on the master branch.

Let me know what you prefer.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#19 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AToJTbWUf29XvrQ5iTdoB0qR67bJXdnKks5qZkjzgaJpZM4JSigv.

[goodboy]

@fchareyron Alright I think we can find a compromise.

I do not think SIPp fails, I rather think that the test is not relevant. The test stops SIPp
before SIPp meets an error condition. It has no reason to generate an error and to produce the error file expected by the test. With the modifications that I proposed, I checked that the error logged in the error file is dated 4 seconds after the first trace of the calldebug file.

The test is relevant it checks that the number of expected files are generated. You added back the error_file which we purposely removed since it is already covered by stderr collection in report.py . You still have never explained why you explicitly need the error_file generated even though you should be able to access the same content fromstderr.

Without fixes proposed in init.py and agent.py at line 127, the
logdir parameter is not treated, traces are always created in /tmp, not
in the expected directory.

Agreed that this is a bug but I don't believe you change solves it fully. Testing this manually I get:

>>> next(pysipp.walk('./scens/', logdir='/var/tmp'))[1].logdir
'/tmp'

Which shows that the Scenario is never properly assigned the logdir even though it is for the Agents...
So your fix does resolve the issue but really the design should be changed.
I've opened #20 to address this.

In report.py, you focus on the loop, and I agree, it is probably not
necessary. But the real fix is 3 lines after. The code must check that
'proc' has a 'streams' attribute before using it. If not, pysipp crashes
in rare cases. My proposal is to create a local stderr just to log the
cause.

The fix you propose calls proc.communicate() a second time after it should have already been called in launch.py. So the only way you should run into the problem you're describing is if you're running the scenario asynchronously with scen(block=False) which is not well tested. So if you can provide a test or script which demonstrates this failure happening in report.py:51 then we can try to get a proper fix for it. To add emphasis: process handling should in no way be done in report.py and instead should be handled in either launch.py or the pysipp_run_protocol hook. I would very much appreciate if you can show how to reproduce this problem so we can get a more correct fix for it.

[goodboy]

@fchareyron I will include both fixes since you are right that the pysipp.__init__.py change does indeed fix the immediate logdir assignment issue.

[goodboy]

@fchareyron I've merge the agreed upon changes to master in f888d3c.

As long as you don't think we need to open any further issues (notably that you wanted the error_file flag passed) I will close this PR and we can address the other fixes in the additional issues I have opened (#20 and #21).

Thanks!

[fchareyron]

Hi

Le 28/07/2016 à 20:22, goodboy a écrit :

@fchareyron https://github.com/fchareyron Alright I think we can
find a compromise.

I do not think SIPp fails, I rather think that the test is not
relevant. The test stops SIPp
before SIPp meets an error condition. It has no reason to generate
an error and to produce the error file expected by the test. With
the modifications that I proposed, I checked that the error logged
in the error file is dated 4 seconds after the first trace of the
calldebug file.

The test is relevant it checks that the number of expected files are
generated. You added back the |error_file| which we purposely removed
since it is already covered by |stderr| collection in |report.py| .
You still have never explained why you explicitly need the
|error_file| generated even though you should be able to access the
same content from|stderr|.

pysipp is part of a RobotFramework library (http://robotframework.org/).

The output streams are manage by RobotFramework. It is probably possible
to collect stderr but I don't want to waste time on that point, because
error file is the natural way to get the info.
But don't focus on that point, you explained how to use scenkwargs to
achieve the purpose. If it works, I implement the solution, if not, I
locally add 'error' in the output list. It is not a problem for me if
pytest fails.

Without fixes proposed in *init*.py and agent.py at line 127, the
logdir parameter is not treated, traces are always created in
/tmp, not
in the expected directory.

Agreed that this is a bug but I don't believe you change solves it
fully. Testing this manually I get:

next(pysipp.walk('./scens/',logdir='/var/tmp'))[1].logdir
'/tmp'

Which shows that the |Scenario| is never properly assigned the logdir
even though it is for the |Agent|s...
So your fix does resolve the issue but really the design should be
changed.
I've opened #20 #20 to address
this.

For the best way to fix issues, I trust you.

In report.py, you focus on the loop, and I agree, it is probably not
necessary. But the real fix is 3 lines after. The code must check that
'proc' has a 'streams' attribute before using it. If not, pysipp
crashes
in rare cases. My proposal is to create a local stderr just to log the
cause.

The fix you propose calls |proc.communicate()| a second time after it
should have already been called in |launch.py|
https://github.com/SIPp/pysipp/blob/master/pysipp/launch.py#L98. So
the only way you should run into the problem you're describing is if
you're running the scenario asynchronously with |scen(block=False)|
which is not well tested. So if you can provide a test or script which
demonstrates this failure happening in report.py:51
https://github.com/SIPp/pysipp/blob/master/pysipp/report.py#L51 then
we can try to get a proper fix for it. To add emphasis: process
handling should in no way be done in |report.py| and instead should
be handled in either |launch.py| or the |pysipp_run_protocol|
https://github.com/SIPp/pysipp/blob/master/pysipp/__init__.py#L206
hook. I would very much appreciate if you can show how to reproduce
this problem so we can get a more correct fix for it.

You state 'streams' is created by proc.communicate(). I agree. But
there are rare cases it is not. 'streams' is not created in the thread
that starts SIPp with popen. There is a short delay between the 2
function calls.
The RobotFramework library I am building is used for load test purposes.
It creates multiple threads handling the same scenario with differents
parameters. The goal is to load asterisk from multiple UAC/AUS pairs
exchanging multiple calls at the same time.
You can imagine that the server that supports the library is busy, and
the delay between the call to popen and the call to proc.communicate()
may vary. If any exception occurs, it is possible that 'streams' is used
before being created.
The patch creates the missing attribute where the exception is risen
when it occurs. It is not an elegant solution, but it works. An other
solution would be to create 'stream' just after calling popen. I am
quite sure you will find a beter solution. If not, I keep mine despite
its defects.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#19 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AToJTaRDgLdLJgV9C9a8t6LMJYXzB63Sks5qaPOBgaJpZM4JSigv.

[fchareyron]

Please, do it!

Le 28/07/2016 à 20:32, goodboy a écrit :

@fchareyron https://github.com/fchareyron I will include both fixes
since you are right that the |pysipp.init.py| change does indeed
fix the immediate |logdir| assignment issue.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#19 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AToJTWuD9n2E1UnaLPt_TKuhjeJXbwyNks5qaPW7gaJpZM4JSigv.

[fchareyron]

OK

Le 28/07/2016 à 21:04, goodboy a écrit :

@fchareyron https://github.com/fchareyron I've merge the agreed upon
changes to master in f888d3c
f888d3c.

As long as you don't think we need to open any further issues (notably
that you wanted the |error_file| flag passed) I will close this PR and
we can address the other fixes in the additional issues I have opened
(#20 #20 and #21
#21).

Thanks!

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#19 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AToJTQfTa91wc7tkbEkU-PcOdVc5vLLdks5qaP0zgaJpZM4JSigv.

[fchareyron]

Hello

You did not include a part of the fix of the logdir issue. Did you miss
it, or is there a reason?

In agent.py at line 130, the provided file name contained in attr is
ignored. The fix just uses attr if provided.

prefix all log file paths

     for name, attr in self.iter_logfile_items():
         setattr(
             self, name, path.join(
                 logdir or self.logdir or tempfile.gettempdir(),

*attr or *"{}_{}".format(self.name, name))
)
self.enable_tracing()

Best regards

Frederic

Le 29/07/2016 à 11:43, CHAREYRON Frederic a écrit :

OK

Le 28/07/2016 à 21:04, goodboy a écrit :

@fchareyron https://github.com/fchareyron I've merge the agreed
upon changes to master in f888d3c
f888d3c.

As long as you don't think we need to open any further issues
(notably that you wanted the |error_file| flag passed) I will close
this PR and we can address the other fixes in the additional issues I
have opened (#20 #20 and #21
#21).

Thanks!

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#19 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AToJTQfTa91wc7tkbEkU-PcOdVc5vLLdks5qaP0zgaJpZM4JSigv.

[goodboy]

@fchareyron I feel like I somehow missed it.
Can you open another issue specifically going over how you expect it to work.

[fchareyron]

Hello, if created #23 xxx_file is not respected in defaults

Best regards

Le 29/08/2016 à 21:54, goodboy a écrit :

@fchareyron https://github.com/fchareyron I feel like I somehow
missed it.
Can you open another issue specifically going over how you expect it
to work.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#19 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AToJTTIrSyKWfH5zgZphWXR2FrRjr48bks5qkzjpgaJpZM4JSigv.