Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -109,20 +109,20 @@ You can also launch multiple multi-UA scenarios concurrently using
non-blocking mode:

```python
scens = []
finalizers = []
Copy link
Member

Choose a reason for hiding this comment

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

Yah much better.

for path, scen in pysipp.walk('path/to/scendirs/root/', proxyaddr=('10.10.8.1', 5060)):
print("Running scenario collected from {}".format(path))
scen(block=False)
scens.append(scen)
finalizers.append(scen(block=False))

# All scenarios should now be running concurrently so we can continue
# program execution...
print("Continuing program execution...")

# Wait to collect all the results
# Calling `scen` returns a finalizer that can be
# used to wait for each scenario with a timeout
print("Finalizing all scenarios and collecting results")
for scen in scens:
scen.finalize()
Copy link
Member

Choose a reason for hiding this comment

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

Yeah not sure what you mean about "breaking the api", you're already doing that with this change afaict.. so you'll have to explain.

for finalizer in finalizers:
finalizer(timeout=10)
Copy link
Member

@goodboy goodboy Aug 4, 2021

Choose a reason for hiding this comment

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

I think we've talked about it in passing but, we might want to move to an explicit method name here. I'm not against .__call__() per say but it does seem a bit too implicit the more we've let this sit.

Definitely something for future work though, no need to address here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hello, are you talking about the ScenarioType having both run and __call__ methods?
the finalizer returned is just a function. Docs are incorrect here thought i will fix them, calling scen returns the finalizer

Copy link
Member

Choose a reason for hiding this comment

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

I'm talking about 2 things:

  • Scenario.run() I think should be the api we encourage using.
  • you dropping .finalize() as part of the runner api.

Right now you've made things arguably more indirected by returning a closure from Scenario.__call__() only when block=False. I wouldn't call this any more functional per say other then that you return a closure instead of the runner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The user should have access to the finalizer only when block=False. It would make much more sense if the finalizer was a method on the Scenario instead of juggling between objects, but that wasn't the case in the existing implementation.

Copy link
Member

Choose a reason for hiding this comment

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

It would make much more sense if the finalizer was a method on the Scenario instead of juggling between objects, but that wasn't the case in the existing implementation.

I like this idea better then returning an anon closure personally.
Is there any reason we can't add this?

It also would tie multi-script-execution lifetime to the scenario object, and for example if one wants to get the results from the last scenario run you just call Scenario.results() or Scenario.finalize() or wtv we decide. This makes the scen a little more future-like which it kind of is given we have non-blocking run support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This solution will change the existing API and be backwards incompatible btw.

We are trying to implement a sync interface to an async functionality.

We can make the Scenario an awaitable to simplify the interface, but this would require the user to run async tests. Or the Scenario can just be a context manager.

Copy link
Member

@goodboy goodboy Aug 9, 2021

Choose a reason for hiding this comment

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

This solution will change the existing API and be backwards incompatible btw.

Not if we make Scenario.__call__() a thing tho right? Also, right now we're documented as scen.finalize() as the api no? I'm not sure how we're breaking anything exactly?

We are trying to implement a sync interface to an async functionality.

Yes which is why this needs thorough thought and ideally more then just our opinions 😂

Scenario can just be a context manager.

This idea i don't mind. If we're going to offer a sync interface it may also make sense to consider spawning trio in another thread and then having the main thread make calls to it with trio.to_thread() and friends.

We should eventually be supporting async code for spawning and scenarios especially to support multi-script cases where a user wants to do cmd restarts, cancels, etc.

```

`scen.finalize()` actually calls a special cleanup function defined in the
Expand Down
23 changes: 6 additions & 17 deletions pysipp/agent.py
Original file line number Diff line number Diff line change
Expand Up @@ -441,25 +441,14 @@ async def arun(
runner=None,
block=True,
):
self._prepared_agents = agents = self.prepare()
self._runner = runner = runner or launch.TrioRunner()

return await launch.run_all_agents(runner, agents, timeout=timeout, block=block)

def finalize(self, *, timeout=DEFAULT_RUNNER_TIMEOUT):
assert (
self._prepared_agents and self._runner
), "Must run scenario before finalizing."
return trio.run(
partial(
launch.finalize,
self._runner,
self._prepared_agents,
timeout=timeout,
)
agents = self.prepare()
self.runner = runner or launch.TrioRunner()

return await launch.run_all_agents(
self.runner, agents, timeout=timeout, block=block
)

def run(self, timeout=180, **kwargs):
def run(self, timeout=DEFAULT_RUNNER_TIMEOUT, **kwargs):
"""Run scenario blocking to completion."""
return trio.run(partial(self.arun, timeout=timeout, **kwargs))

Expand Down
19 changes: 14 additions & 5 deletions pysipp/launch.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@
import subprocess
import time
from . import utils
from pprint import pformat
from collections import OrderedDict, namedtuple
from functools import partial
from pprint import pformat

import trio

Expand Down Expand Up @@ -149,18 +150,19 @@ def clear(self):
self._procs.clear()


async def run_all_agents(runner, agents, timeout=180, block=True):
async def run_all_agents(runner, agents, timeout, block=True):
"""Run a sequencec of agents using a ``TrioRunner``."""

try:
await runner.run((ua.render() for ua in agents), timeout=timeout)
if block:
await finalize(runner, agents, timeout)
return runner
return await finalize(runner, agents, timeout)
else:
return finalizer(runner, agents)
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I just don't see how this is beneficial.

Instead of returning the runner now we're returning a closure that calls trio.run() and that closure seems mostly to be a function created solely for the purposes of having it be a function (aka an object with .__call__())

I actually find this somewhat hard to follow and not any more functional then the original method which does this same thing.

Copy link
Contributor Author

@kontsaki kontsaki Aug 4, 2021

Choose a reason for hiding this comment

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

What is the benefit of exposing an implementation detail like the runner to the user? The downside is that it leaves more space for the user to misuse the API.

except TimeoutError as terr:
# print error logs even when we timeout
try:
await finalize(runner, agents, timeout)
return await finalize(runner, agents, timeout)
except SIPpFailure as err:
assert "exit code -9" in str(err)
raise terr
Expand All @@ -178,3 +180,10 @@ async def finalize(runner, agents, timeout):
raise SIPpFailure(msg)

return cmds2procs


def finalizer(runner, agents):
Copy link
Member

@goodboy goodboy Aug 4, 2021

Choose a reason for hiding this comment

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

Yeah, I don't see how this gives anything more functional honestly.

Instead of a runner (which also defines .__call__() and thus is duck-typed like a function) you return a closure function (which could have just as easily with a partial(trio.run, partial(finalize, runner, agents, timeout=timeout)) which seems nearly to be a no-op indirection level).

What's the motivation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the API is cleaner and less error prone as the runner is not exposed to the user anymore which means fewer chances of reaching invalid states. Functional as in less object oriented.

Copy link
Member

Choose a reason for hiding this comment

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

the API is cleaner and less error prone as the runner is not exposed to the user anymore which means fewer chances of reaching invalid states.

Not sure how that's possible to reach invalid states but yeah I guess in theory?

If we don't have a runner and only a simple function there there's also no future option for adding cancellation support, which is kinda what trio is all about. I don't want to over-design now or anything but making this the api is yes maybe simpler (as much as .__call__() vs. .finalize() is) but it may be somewhat limiting down the road?

Functional as in less object oriented.

Because we aren't returning a runner?
Calling TrioRunner.finalize() vs. lambda.__call__() i'm not really seeing as being any more or less objecty in python but 🤷🏼. I still don't understand why you need this function block, why not just return the partial on trio.run() if you wanted this; heck it could be a lambda?

def with_timeout(timeout):
return trio.run(partial(finalize, runner, agents, timeout=timeout))

return with_timeout
2 changes: 1 addition & 1 deletion tests/test_launcher.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

def run_blocking(runner, agents):
assert not runner.is_alive()
trio.run(run_all_agents, runner, agents)
trio.run(run_all_agents, runner, agents, 10)
assert not runner.is_alive()
return runner

Expand Down
16 changes: 8 additions & 8 deletions tests/test_stack.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,29 +73,29 @@ def test_proxyaddr_with_scendir(scendir):
def test_sync_run(scenwalk):
"""Ensure all scenarios in the test run to completion in synchronous mode"""
for path, scen in scenwalk():
runner = scen(timeout=6)
for cmd, proc in runner._procs.items():
finalizer = scen(timeout=6)
for cmd, proc in finalizer.items():
assert proc.returncode == 0


def test_async_run(scenwalk):
"""Ensure multiple scenarios run to completion in asynchronous mode."""
finalizers = []
for path, scen in scenwalk():
finalizers.append((scen, scen(block=False)))
for _, scen in scenwalk():
finalizers.append(scen(block=False))

# collect all results synchronously
for scen, finalizer in finalizers:
for cmd, proc in scen.finalize(timeout=6).items():
for finalizer in finalizers:
for cmd, proc in finalizer(timeout=6).items():
assert proc.returncode == 0


def test_basic(basic_scen):
"""Test the most basic uac <-> uas call flow"""
assert len(basic_scen.agents) == 2
# ensure sync run works
runner = basic_scen()
assert not runner.is_alive()
basic_scen()
assert not basic_scen.runner.is_alive()


def test_unreachable_uas(basic_scen):
Expand Down